Bug Hunting Session
Bug 96896 - Writer crashes when frame title is changed via API and user closes print preview
Summary: Writer crashes when frame title is changed via API and user closes print preview
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Writer (show other bugs)
Version:
(earliest affected)
5.0.4.2 release
Hardware: All Windows (All)
: medium normal
Assignee: Not Assigned
URL:
Whiteboard: target:5.2.0 target:5.1.3
Keywords: difficultyInteresting, easyHack, haveBacktrace, skillCpp
Depends on:
Blocks: 96044 98834
  Show dependency treegraph
 
Reported: 2016-01-05 00:29 UTC by Rodrigo Castanheira
Modified: 2017-02-14 08:58 UTC (History)
8 users (show)

See Also:
Crash report or crash signature:


Attachments
Open a document and change frame title via API (VBScript) (1.02 KB, text/plain)
2016-01-05 00:29 UTC, Rodrigo Castanheira
Details
Backtrace generated with WinDbg(X64) - LO 5.1.0.3 (x86), Win10 64 bit (18.95 KB, text/plain)
2016-03-04 03:32 UTC, Rodrigo Castanheira
Details
ProposedPatch (1.77 KB, patch)
2016-03-16 17:48 UTC, akash96j
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Rodrigo Castanheira 2016-01-05 00:29:45 UTC
Created attachment 121721 [details]
Open a document and change frame title via API (VBScript)

If a document is opened in Writer 5.0.4.2 (Windows 10) and the corresponding frame title is changed via API, Writer stops responding when user closes print preview. This happens in both edit and read-only modes. To reproduce, see attached file.

I've observed similar behavior in Writer 4.4.7.2 (Windows 7), but only when document is opened in read-only mode. No crash occurs if frame title is changed while in edit mode.
Comment 1 Rodrigo Castanheira 2016-01-05 17:45:39 UTC
Tested 4.4.7.2 again and it stops responding both in edit and read-only modes. While 5.0.4.2 crashes when user closes print preview, 4.4.7.2 crashes when user closes the window.
Comment 2 Oliver Brinzing 2016-01-06 15:38:13 UTC
Maybe there is a workaround for this problem:
https://bz.apache.org/ooo/show_bug.cgi?id=117501

XComponent document = xComponentLoader.loadComponentFromURL(url,"_blank",0,mProps);
XModel xModel = UnoRuntime.queryInterface(XModel.class, document);
XTitle xTitle = (XTitle) UnoRuntime.queryInterface(XTitle.class, xModel);
xTitle.setTitle("Hello World");
Comment 3 Rodrigo Castanheira 2016-01-11 23:43:32 UTC
Thanks, workaround works at least in 5.0.4.2.
However, when in read-only mode, the expression "(read-only)" is automatically appended to the title only until you select print preview. After the first preview, the expression "(read-only)" is replaced by a " : 3" (as is also the case with edit mode).
Comment 4 Yousuf Philips (jay) (retired) 2016-03-02 20:34:37 UTC
Hi castanheira,

Unfortunately i dont have the ability to reproduce the crash, so can you please follow the instructions provided at this link < https://wiki.documentfoundation.org/How_to_get_a_backtrace_with_WinDbg > and submit a backtrace. That way a developer can look into the underlying reason of the crash.
Comment 5 Rodrigo Castanheira 2016-03-04 03:32:25 UTC
Created attachment 123218 [details]
Backtrace generated with WinDbg(X64) - LO 5.1.0.3 (x86), Win10 64 bit
Comment 6 Rodrigo Castanheira 2016-03-04 03:34:51 UTC
Hi Yousuf, backtrace submitted, hope it helps. Thanks.
Comment 7 Yousuf Philips (jay) (retired) 2016-03-04 04:53:00 UTC
Hi castanheira,

Thanks for the backtrace. Hopefully one of devs will be able to look at it and track down the cause.
Comment 8 Michael Meeks 2016-03-04 09:43:35 UTC
Looks like a silly infinite recursion:

mergedlo!SfxBaseController::getTitle+0x4f
mergedlo!framework::TitleHelper::impl_updateTitleForController+0x239
mergedlo!framework::TitleHelper::impl_updateTitle+0x19d
mergedlo!framework::TitleHelper::getTitle+0x96
mergedlo!SfxBaseController::getTitle+0x4f

It is possible that this fixes it:

--- a/framework/source/fwe/helper/titlehelper.cxx
+++ b/framework/source/fwe/helper/titlehelper.cxx
@@ -417,7 +417,8 @@ void TitleHelper::impl_updateTitleForController (const css::uno::Reference< css:
         xModelTitle.set(xController, css::uno::UNO_QUERY);
     if (xModelTitle.is ())
     {
-        sTitle.append      (xModelTitle->getTitle ());
+        if (!init) // avoid recursion.
+            sTitle.append      (xModelTitle->getTitle ());
         if ( nLeasedNumber > 1 )
         {
             sTitle.append (" : ");

But that needs testing, and we need a unit test for it - which we should add to 

framework/complex/ModuleManager/CheckXModuleManager.java

or perhaps a copy of that - operating on the frame there.
 => turning into an easy hack =)
Comment 9 akash96j 2016-03-10 13:06:03 UTC
(In reply to castanheira from comment #3)
> Thanks, workaround works at least in 5.0.4.2.
> However, when in read-only mode, the expression "(read-only)" is
> automatically appended to the title only until you select print preview.
> After the first preview, the expression "(read-only)" is replaced by a " :
> 3" (as is also the case with edit mode).

Hi,
I am trying to solve this bug.
As pointed by Michael this bug is originating in
/framework/source/fwe/helper/titlehelper.cxx

I believe the "read-only" is getting replaced by ":3" due to these line in the same function block:
sTitle.append(" : ");
sTitle.append    ((::sal_Int32)nLeasedNumber);

My understanding of this task is then: 
1. Prevent the crash
2. Title should remain as "read-only" if "read-only". The title should not have a colon followed by the number. Is this correct?

Regards.
Comment 10 Rodrigo Castanheira 2016-03-11 02:12:23 UTC
Hi akash96j,

A colon followed by a number is appended to the window title when a new window of the same document is opened (Window -> New window). If one or more of these multiple views is closed and you open another one, Libreoffice uses the least available number for its title. When edit mode is toggled (Ctrl+Shift+M), the expression "(read-only)" is added or removed from the title of each window. I think that this is expected behaviour.

However, when print preview is selected, the numbering of windows behaves oddly. To reproduce, please follow these steps (LO 5.1.0.3, Win10):

CASE A - MANUALLY OPENED DOCUMENT
1) Create and open a new Writer document. There is no colon and number in window title (ok).
2) Toggle edit mode with Ctrl+Shift+M. "(read-only)" is appended to window title (ok).
3) Select Print Preview. "(read-only) : 2" is appended to window title. Ok, but it's a bit weird as the previous window is not visible anymore, so why number the preview as "2"?
4) Close Print Preview. "(read-only) : 3" is shown in window title. That's strange and confusing. Where is the original, unnumbered window?

CASE B - USING API TO OPEN DOCUMENT AND CHANGE WINDOW TITLE
1) On the attached VBScript, uncomment lines 15 and 16 and replace line 21 with oDoc.Title = "My Title".
2) Run the modified script to create a new Writer document, open it in read-only mode and change window title. "My Title (read-only)" is the window title (ok).
3) Select Print Preview. ": 2" is appended to window title, but the "(read-only)" expression mysteriously disappears. This seems to be a bug.
4) Close Print Preview. ": 3" is shown in window title without any "(read-only)" expression, as document is now unexpectedly in edit mode.

CASE C - USING API TO OPEN DOCUMENT WITHOUT CHANGING WINDOW TITLE
1) On the attached VBScript, uncomment lines 15 and 16 and delete lines 18 to 21.
2) Run the modified script to create a new Writer document and open it in read-only mode. "(read-only)" is appended to window title (ok).
3) Select Print Preview. "(read-only) : 2" is appended to window title (ok).
4) Close Print Preview. "(read-only) : 3" is shown in window title. Just the same odd numbering behaviour as above.

So, after closing print preview, the original, unnumbered window becomes a ": 3" window. Additionally, if the window title of a read-only document is changed using API, print preview puts the document in edit mode.
Comment 11 akash96j 2016-03-12 19:02:41 UTC
Hi castanheira,

First of all thanks for your explanation of the bug. It was really helpful.
I have been studying the gdb stack-traces, trying to understand the cause of the bug. I have made a few observations. These observations relate less to the original title of the bug but are more related to your cases.
Tested on ubuntu 5.2 dev build
1. Case 1,3: it doesn't matter if we switch to read only mode or not. When switching to print preview mode and back the title changes from :2 to :3 .

2. These cases can be created in linux build as well. Also it is not restricted to only writer but also calc. All other modules don't have a print preview option.Therefore this a bug in all platforms across multiple modules.

3. The creation of title seems to be highly recursive procedure. I was counting 8 to 9 levels of recursion for each title change. The title is built in three parts. The frame,controller and model title are concatenated. The problem in setting the title from the api is that the function setting the title for the title returns without setting the title string if an external title is present. The api is most likely changing only the model part of the title. 'read-only' is added in the model title but if the modeltitle is set from outside then the model part of the title string is not set internally. I am unable to recreate this part in ubuntu and I am unable to build on windows. Can you please confirm one thing for me? In case 2 after step 4 are you actually able to edit the doc or the read-only in not present in the title but the document is in read-only mode?

4. The problem of wrong numbers occurs due to the fact that when a component is detached from the frame(frameAction_COMPONENT_DETACHED) event, it never releases its lease number. Even though the controller and the window for the component are disposed, the disposing function in titlehelper is never called.
In fact the title for a detaching component is generated even though it will never be used. I don't understand why this is happening.

5. When a new component reattaches to the frame(frameAction_COMPONENT_REATTACHED) the lease number goes up. But this would mean that continuously switching between print preview and default mode should increase the number. Again this is not the case even though there is no release of the leased number. I am unable to figure out why this is happening even though the XUntitledNumbers function leaseNumber is being called by the same XNumber object everytime. Can any dev help me on this?
But according to me this last statement is not critical to solving the bug.

To solve the bug, I think that when detaching a component its lease number should be released and there is no need to regenerate its title i.e. in the class framework::TitleHelper::frameAction frameAction_COMPONENT_DETACHING should be handled separately. I seek advice on how to handle this case separately. Should the leasedNumber be released in a separate function? Because the current disposing function in the titlehelper only works on a dispose event and dispose event is not created when a component is detached.(framework/source/services/frame.cxx).
A couple of more questions:
1. Is a mutex lock automatically released when is goes out of scope without clearing the lock?
2. Can anyone point me to some documentation of the model-control-frame model of application. I am currently refering to this: https://wiki.openoffice.org/wiki/Documentation/DevGuide/OfficeDev/Frame-Controller-Model_Paradigm_in_OpenOffice.org

Also... the original bug can't be solved by using
if (!init) // avoid recursion.
    sTitle.append      (xModelTitle->getTitle ());
This will break the title because in a component reattachment event impl_updateTitleForController is called with a true value. This is because in a reattachment the controller and window are new so the controller title is never set before.If we only set the title when init is false then on changing views the controller part of the title will disappear(tested on ubuntu).
This bug ,acc to me, is happening due to the fact that a controller title includes a model title. In impl_updateTitleForController there is a call to getTitle for the model. But somehow in the impl_updateTitle function the component is being always identified as a controller. Thus the recursion.
My understanding of some parts may be wrong. Please correct anything you feel is not right so I can better understand the problem.
Regards.
Comment 12 Michael Meeks 2016-03-12 21:39:52 UTC
Wow =) I just love the analysis from you guys; TBH at this point - I suspect that no-one else in the project has as deep an understanding of you guys of this code as is shown in the analysis here.

In terms of proceeding; what I would suggest is:

a) fix the bug how you think best (akash)
b) write unit tests for the document title in each of these corner-case circumstances we've found - which will help capture your experience here for the benefit of future fixers.
c) - lets just get that committed ...
d) if possible make this far less recursive (ie. faster & cleaner ;-)

> Can any dev help me on this?

git grep -10 leaseNumber

shows a number of comphelper/ hits - I guess the lease is expired and then re-used or somesuch (but having not read it - I have far less insight than you as to what is going on ) - it strikes me Stephan may have a view on framework/ pieces though.

HTH.
Comment 13 Rodrigo Castanheira 2016-03-13 04:42:36 UTC
Hi,

In reply to akash96j from comment #11 ("Can you please confirm one thing for me? In case 2 after step 4 are you actually able to edit the doc or the read-only in not present in the title but the document is in read-only mode?"):

I am actually able to edit the doc after step 4 in case 2. Surprisingly, I am also able to edit the doc before step 3! I haven't noticed that before. When a new doc is opened using loadComponentFromURL("private:factory/swriter","_blank",0,OpenOptions) and OpenOptions has a value "ReadOnly" = True, the expression "(read-only)" is appended to the window title, but document is not put in read-only mode. It is not possible to toggle edit mode using Ctrl+Shift+M (grayed out under Edit menu). This behaviour is the same whether window title is changed or not.

However, when an existing document is opened using loadComponentFromURL("file:///C:/Users/Public/existing.odt","_blank",0,OpenOptions) and OpenOptions has a value "ReadOnly" = True, the expression "(read-only)" is appended to the window title and, as expected, document actually opens in read-only mode. Then, if window title is changed using API and print preview is selected, the expression "(read-only)" dissapears but document remains in read-only mode. From now on, no matter how many times you toggle edit mode using Ctrl+Shift+M, the expression "(read-only)" is never appended to window title.

To further investigate the behaviour of loadComponentFromURL with the URL "private:factory/swriter", I've removed the value "ReadOnly" from OpenOptions. Document is opened in edit mode as expected, but Ctrl+Shift+M is grayed out under Edit menu until you save document.

So, it seems that a document cannot be put in read-only mode before it is saved to disk. If so, loadComponentFromURL with URL "private:factory/swriter" and OpenOptions "ReadOnly"=True should be considered unsupported. Nevertheless, Libreoffice appends "(read-only)" to the window title...
Comment 14 akash96j 2016-03-14 04:00:45 UTC
Hi,
(In reply to castanheira from comment #13)

> I am actually able to edit the doc after step 4 in case 2. Surprisingly, I
> am also able to edit the doc before step 3! I haven't noticed that before.
> When a new doc is opened using
> loadComponentFromURL("private:factory/swriter","_blank",0,OpenOptions) and
> OpenOptions has a value "ReadOnly" = True, the expression "(read-only)" is
> appended to the window title, but document is not put in read-only mode. It
> is not possible to toggle edit mode using Ctrl+Shift+M (grayed out under
> Edit menu). This behaviour is the same whether window title is changed or
> not.
> To further investigate the behaviour of loadComponentFromURL with the URL
> "private:factory/swriter", I've removed the value "ReadOnly" from
> OpenOptions. Document is opened in edit mode as expected, but Ctrl+Shift+M
> is grayed out under Edit menu until you save document.
> So, it seems that a document cannot be put in read-only mode before it is
> saved to disk. If so, loadComponentFromURL with URL
> "private:factory/swriter" and OpenOptions "ReadOnly"=True should be
> considered unsupported. Nevertheless, Libreoffice appends "(read-only)" to
> the window title...
That is correct. Doc needs to be saved before read-only mode can be toggled.
When trying to switch to read-only mode in a new doc I even get a dialog asking me to save the doc first. Perhaps a fix for this,as you also mention, would be: If a doc is opened with a factory/swriter url then read-only mode should be false only.

> However, when an existing document is opened using
> loadComponentFromURL("file:///C:/Users/Public/existing.odt","_blank",0,
> OpenOptions) and OpenOptions has a value "ReadOnly" = True, the expression
> "(read-only)" is appended to the window title and, as expected, document
> actually opens in read-only mode. Then, if window title is changed using API
> and print preview is selected, the expression "(read-only)" dissapears but
> document remains in read-only mode.
Thanks for confirming this. I was expecting this only as, you would agree with me, title change will not effect the actual mode of the document. This then is the problem related to the m_bExternalTitle parameter for the impl_updateTitleForModel function. 

>Wow =) I just love the analysis from you guys; TBH at this point - I suspect >that no-one else in the project has as deep an understanding of you guys of >this code as is shown in the analysis here.
:)

> Can any dev help me on this?
>git grep -10 leaseNumber
>shows a number of comphelper/ hits - I guess the lease is expired and then >re-used or somesuch (but having not read it - I have far less insight than you >as to what is going on ) - it strikes me Stephan may have a view on framework/ >pieces though.
Thanks for pointing this out.I have figured it out =).
In comphelper/source/misc/numberedcollection.cxx the leaseFunction maintains a hash with component(a weak reference to it) and its leaseNumber. Now this component reference doesn't match with the reference of the controller passed to the impl_updateTitleForController method. Because a weak reference is being obtained to the original controller and being passed as the component reference. Hence, even when the original controller is disposed, the hash table entry is not deleted as it is maintaining a reference to a reference. It only gets deleted later. Hence the number changes between 2 and 3.

>In terms of proceeding; what I would suggest is:

>a) fix the bug how you think best (akash)
At this moment I am pretty sure on how the bug has to be solved. In my gdb session I am able to recreate the release of the number by a controller and the title behaves the way in the way it should. But I am facing a issue:

1. A frame detach event, obviously, has a frame as the source. It is possible to get the controller related to the frame as well. When the frame event occurs, we have no data related to the controller. And if we try to get the data of the controller, we lose information about the type of the event that has caused us to get the data. Data such as numbered collection is also needed in all other cases. So there is no way to distinguish between the two.

2. When a new component is being set in the frame, the old controller is disposed via dispose(). Then perhaps we could call a function in this function to dispose the title. But the problem is that the controller class stores its attributes in a XTitle reference for title related things. And within the XTitle class there are only two function to get the string and to set it. Nothing else. And we need to get the numbered collection and the component reference. Then we need to obtain a weak reference to the component.

3. Then I propose to edit the XTitle class. Either to add a new function. Or to modify the setTitle function which takes a parameter bool dispose false by default but can be set to true to dispose the string.I seek your advice on this method. 

4. There is another method which I explored. When a frame disposes it sends a event to its listeners. The titleHelper is one of this listener and hence recieves this event and disposes the frames lease number. Similarly when a controller disposes it also generates a event. But I was unable to find and function which would register to controller Events. If there was such a function then the title dispose would take place by itself. I saw that SfxClipboard receives controller dispose events. But was not able to find how it registered. Perhaps I will take a second look... 

>b) write unit tests for the document title in each of these corner-case circumstances we've found - which will help capture your experience here for the benefit of future fixers.
>c) - lets just get that committed ...
I am also eager to commit :). I just want to make sure that my testing is thorough so the application is more robust. 
>d) if possible make this far less recursive (ie. faster & cleaner ;-)
I think there will be recursion due to the method in which a title is created. It just makes debugging difficult and tedious. Maybe after patching the bug, I will see how the process can be improved.
Comment 15 akash96j 2016-03-14 20:24:54 UTC
In addition to my previous comment:
I have found out that it is not possible to edit the XTitle class as it is generated from a .idl file. The main problem is this:
The data related to the controller is stored in a XTitle reference. But Xtitle has only two functions to set or get title. Now when a controller is disposing, I am able to get its data in a XTitle reference but am not able to call a dispose function. Should I extend the XTitle class just for this one case?
Comment 16 Michael Meeks 2016-03-14 21:14:44 UTC
Hi there,

Soo ... changing XTitle seems like a poor idea; presumably it is a published interface and so cannot be changed. It is possible to add a new interface to this that we can get via a queryInterface - and the associated Reference<> method that will do a cast $ git grep QUERY_THROW eg.

Perhaps easier, we can use a dynamic_cast<> to get the implementation of the XTitle interface - as a class Frame (actually that's private to the frame.cxx) - but perhaps another non-UNO shared interface; but I guess at this point that's also painful.

> And within the XTitle class there are only two function to get the string
> and to set it. Nothing else. And we need to get the numbered collection
> and the component reference. Then we need to obtain a weak reference to
> the component.

And I'm really glad its you not me in there ;-) perhaps Stephan has some better ideas.
Comment 17 Maxim Monastirsky 2016-03-14 22:45:32 UTC
(In reply to akash96j from comment #15)
> The main problem is this:
> The data related to the controller is stored in a XTitle reference. But
> Xtitle has only two functions to set or get title. Now when a controller is
> disposing, I am able to get its data in a XTitle reference but am not able
> to call a dispose function. Should I extend the XTitle class just for this
> one case?
No, but you can make sure that the object that implements XTitle also implements css::lang::XComponent, and then you can do something like this:

css::uno::Reference< css::lang::XComponent > xComponent( xTitleHelper, css::uno::UNO_QUERY );
    if ( xComponent.is() )
        xComponent->dispose();

Or take a different approach:

(In reply to akash96j from comment #14)
> 4. There is another method which I explored. When a frame disposes it sends
> a event to its listeners. The titleHelper is one of this listener and hence
> recieves this event and disposes the frames lease number. Similarly when a
> controller disposes it also generates a event. But I was unable to find and
> function which would register to controller Events.
Well, SfxBaseController seems to implement css::lang::XComponent, so you can use XComponent::addEventListener for that.
Comment 18 akash96j 2016-03-16 17:47:21 UTC
Hi,

I have solved the bug.

Now, just want to make clear some details about this bug(warning:wall of text ahead):
Although the original description of the bug is about writer crashing when the title is changed from the api, the discussion revealed two new bugs. So in all there were three bugs:
1. Writer crashes when title is changed from api.
2. Wrong numbers showing up in the title when changing from print preview and back.
3. Even though read-only mode is not supported for an unsaved doc, writer appends read-only when a document is opened with factory/swriter url.

So here is how the patch is working for each bug:

Bug 1. This is caused due to two lines (commented) in the patch. Some observations: The controller appends to its title, the title of the model.
When a model disposes, it calls disposing in TitleHelper to release its resources.
The call to dispose calls impl_sendTitleChangeEvent. Since a model title is a subtitle for a controller, the controller calls for an update in its title.
The controller calls impl_updateTitleForController. Then in lines:
css::uno::Reference< css::frame::XTitle > xModelTitle(xController->getModel (), css::uno::UNO_QUERY);
xModelTitle reference is NULL!! because the model is disposing...
if (!xModelTitle.is ())
       xModelTitle.set(xController, css::uno::UNO_QUERY);
This line now calls the getTitle for a controller. and hence this recursion:
framework::TitleHelper::impl_updateTitleForController+0x81
framework::TitleHelper::impl_updateTitle+0x19d
framework::TitleHelper::getTitle+0x96
SfxBaseController::getTitle+0x4f

I don't know why these lines are here. I set a breakpoint at these lines and ran through a series of events in writer. Then I checked the number of hits on the breakpoint. Hits only occurred when the model was disposing.
Also peculiar is what follows after: (in short)
if (!xModelTitle.is ())
       xModelTitle.set(xController, css::uno::UNO_QUERY);
if (xModelTitle.is())
{//code}
else
{//code}
Now if the xModelTitle is an empty reference, then it is made to refer to the controller calling it. Then again there is a if clause which will always be taken because we just set the reference!! There is also an else part which will never be executed. Maybe someone can tell us what these lines were meant to do..

Although it seems this is too much work just to break an infinite recursion, it is not. I tried many variations all of which broke the loop, but also broke some other part. One of this (appears in the patch) was that the model was sending a title change event always, even if the title was not changed. Now when the model was disposing, there was no title change, but still the controller starts to update its subTitle and falls into recursion. In the original code there is a reason for this which doesn't really help.
//can be changed after shared mode is supported as per UNO api
So, I didn't change this line.(although I tried experimenting and it broke something else in the title).

Bug 2. As mentioned in earlier discussions, the XController was never releasing its leasedNumber. This was identified early in the gdb sessions. But the real problem was how to get the controller to release its resources(see comment 14).
I wanted to use the existing framework rather than writing new functions for this case. I had observed that the controller sends a dispose event. So I registered TitleHelper for it. The problem here was that casting XTitle into a XListenerEvent reference directly is an ambiguous cast. So I have defined the upcasting path:
xController->addEventListener(static_cast<css::lang::XEventListener*> (static_cast< css::frame::XFrameActionListener* > (this)));
We can use dynamic_cast also but I think static will work in this case.
This easily fixes the problem as controller dispose event disposes the related controllers resources. Also it requires minimum editing to the existing code base.

Bug 3. It is required that a document be saved to toggle read-only mode. But writer, if called from the api with a read-only option, loads it into edit mode but still appends 'read-only' to the tittle. I went through the loadenv class for this. And there url of types private:factory are given a CAN_BE_LOADED classification. Because the content can be loaded in edit mode. 
So should a exception be thrown (INVALID_MEDIADESCRIPTOR) or continue to load in edit mode, but set read-only property as false? 
imho nothing should happen without the knowledge of the programmer. So a exception should be thrown. And ideally a file save dialog(fileDialogHelper.cxx) should be opened after load environment is initialized. Which opens a lot of other questions like when and how to throw the dialog etc.
But I think that can be made in a different commit.

I wrote this up so that in the future, someone else need not spend much time figuring out what is happening and help him/her to hunt down the bug faster. Because I feel that their maybe a nasty edge-case hiding somewhere.
Please review the patch.
Also I plan to do the following:
1. Write unit tests for all the test cases I found.
2. titlehelper.cxx could use some documentation. So I plan to add that.
Comment 19 akash96j 2016-03-16 17:48:41 UTC
Created attachment 123633 [details]
ProposedPatch
Comment 20 akash96j 2016-03-16 17:54:57 UTC
(In reply to Rodrigo Castanheira from comment #13)
Hi castan,
> I am actually able to edit the doc after step 4 in case 2. Surprisingly, I
> am also able to edit the doc before step 3! I haven't noticed that before.
> When a new doc is opened using
> loadComponentFromURL("private:factory/swriter","_blank",0,OpenOptions) and
> OpenOptions has a value "ReadOnly" = True, the expression "(read-only)" is
> appended to the window title, but document is not put in read-only mode. It
> is not possible to toggle edit mode using Ctrl+Shift+M (grayed out under
> Edit menu). This behaviour is the same whether window title is changed or
> not.
I found some documentation on this:
Read-Only
Tells whether a document should be loaded in a (logical) readonly or in read/write mode.By default the loaded content decides what to do: if its UCB content supports a "readonly" property, the logical open mode depends on that, otherwise it will be read/write. This is only a UI related property, opening a document in read only mode will not prevent the component from being modified by API calls, but all modifying functionality in the UI will be disabled or removed. 

> However, when an existing document is opened using
> loadComponentFromURL("file:///C:/Users/Public/existing.odt","_blank",0,
> OpenOptions) and OpenOptions has a value "ReadOnly" = True, the expression
> "(read-only)" is appended to the window title and, as expected, document
> actually opens in read-only mode. Then, if window title is changed using API
> and print preview is selected, the expression "(read-only)" dissapears but
> document remains in read-only mode. From now on, no matter how many times
> you toggle edit mode using Ctrl+Shift+M, the expression "(read-only)" is
> never appended to window title.
According to the implementation of TitleHelper, this is also correct behaviour.
If external title is found it won't be updated internally.

I hope all undefined behaviours have been explained. I have attached a patch. I hope it solves the problem.
Comment 21 Rodrigo Castanheira 2016-03-17 01:48:39 UTC
Hi akash96j,

Thank you for your thoroughly analysis and effort resolving these bugs. I'm sorry that I don't have the resources nor the knowledge to generate a patched build to help verify the solution.
Regards.
Comment 22 Michael Meeks 2016-03-17 13:36:23 UTC
akash96j - this work looks extremely thorough and helpful =)

Thank you !

Please can you do the following:

a) split each of your patches that fixes a bug into a separate commit; it also helps to commit them with some separation too - to help later QA people bibisect to the right one.

b) include your rational - a few lines; and tdf#1234 for the bug in the first-line to make it easy to reference them.

c) include a unit test for each category of fix; it should be possibly to adapt an existing test here I think - but at least, where there is significant unclarity about what it is supposed to do, tests are even more vital - so we can be sure we don't regress in future.

Otherwise - the fixes look plausible to me; do they pass 'make check' =) if so, with the above; lets get them in.

Great work & analysis; sorry it took a while to get to review this, and sorry that this was in no way an 'Easy' easy-hack =)
Comment 23 Stephan Bergmann 2016-03-23 12:58:52 UTC
(In reply to Rodrigo Castanheira from comment #0)
> Created attachment 121721 [details]
> Open a document and change frame title via API (VBScript)

The corresponding code in LO Basic is:

> Sub Main
> Dim oDoc
> Dim OpenOptions()
> Set oDoc = StarDesktop.loadComponentFromURL("private:factory/swriter", "_blank", 0, OpenOptions)
> Dim oCtrl, oFrame
> Set oCtrl = oDoc.CurrentController
> Set oFrame = oCtrl.Frame
> oFrame.Title = "My Title"
> Msgbox "KEEP THIS DIALOG OPEN. On Writer document, select File -> Print preview, then close preview."
> oDoc.Close (True)
> Set oDoc = Nothing
> End Sub
Comment 24 Stephan Bergmann 2016-03-23 13:05:52 UTC
...and the relevant bottom of the infinite recursion is:

> ...
> framework::TitleHelper::impl_updateTitle(bool) (this=0x7fffa8a6fd98, init=true) at framework/source/fwe/helper/titlehelper.cxx:307
> framework::TitleHelper::getTitle() (this=0x7fffa8a6fd98) at framework/source/fwe/helper/titlehelper.cxx:109
> non-virtual thunk to framework::TitleHelper::getTitle() () at framework/source/fwe/helper/titlehelper.cxx:92
> SfxBaseController::getTitle() (this=0x7fffbc4d0600) at sfx2/source/view/sfxbasecontroller.cxx:1515
> non-virtual thunk to SfxBaseController::getTitle() () at sfx2/source/view/sfxbasecontroller.cxx:1512
> framework::TitleHelper::impl_updateTitleForController(com::sun::star::uno::Reference<com::sun::star::frame::XController> const&, bool) (this=0x7fffa8a6fd98, xController=uno::Reference to (SwXTextView *) 0x7fffbc4d0628, init=true) at framework/source/fwe/helper/titlehelper.cxx:420
> framework::TitleHelper::impl_updateTitle(bool) (this=0x7fffa8a6fd98, init=true) at framework/source/fwe/helper/titlehelper.cxx:307
> framework::TitleHelper::getTitle() (this=0x7fffa8a6fd98) at framework/source/fwe/helper/titlehelper.cxx:109
> non-virtual thunk to framework::TitleHelper::getTitle() () at framework/source/fwe/helper/titlehelper.cxx:92
> SfxBaseController::getTitle() (this=0x7fffbc4d0600) at sfx2/source/view/sfxbasecontroller.cxx:1515
> non-virtual thunk to SfxBaseController::getTitle() () at sfx2/source/view/sfxbasecontroller.cxx:1512
> framework::TitleHelper::impl_updateTitleForController(com::sun::star::uno::Reference<com::sun::star::frame::XController> const&, bool) (this=0x7fffa8a6fd98, xController=uno::Reference to (SwXTextView *) 0x7fffbc4d0628, init=false) at framework/source/fwe/helper/titlehelper.cxx:420
> framework::TitleHelper::impl_updateTitle(bool) (this=0x7fffa8a6fd98, init=false) at framework/source/fwe/helper/titlehelper.cxx:307
> framework::TitleHelper::titleChanged(com::sun::star::frame::TitleChangedEvent const&) (this=0x7fffa8a6fd98, aEvent=...) at framework/source/fwe/helper/titlehelper.cxx:169
> framework::TitleHelper::impl_sendTitleChangedEvent() (this=0x7fffcef859c8) at framework/source/fwe/helper/titlehelper.cxx:280
> framework::TitleHelper::disposing(com::sun::star::lang::EventObject const&) (this=0x7fffcef859c8, aEvent=...) at framework/source/fwe/helper/titlehelper.cxx:258
> cppu::OInterfaceContainerHelper::disposeAndClear(com::sun::star::lang::EventObject const&) (this=0x7fffbc0e75c8, rEvt=...) at cppuhelper/source/interfacecontainer.cxx:304
> cppu::OMultiTypeInterfaceContainerHelper::disposeAndClear(com::sun::star::lang::EventObject const&) (this=0x5a72c28, rEvt=...) at cppuhelper/source/interfacecontainer.cxx:477
> SfxBaseModel::dispose() (this=0x7ffff7ec0038) at sfx2/source/doc/sfxbasemodel.cxx:762
> SwXTextDocument::dispose() (this=0x7ffff7ebff08) at sw/source/uibase/uno/unotxdoc.cxx:610
> SfxBaseModel::close(unsigned char) (this=0x7ffff7ec0038, bDeliverOwnership=1 '\001') at sfx2/source/doc/sfxbasemodel.cxx:1403
> SwXTextDocument::close(unsigned char) (this=0x7ffff7ebff08, bDeliverOwnership=1 '\001') at sw/source/uibase/uno/unotxdoc.cxx:618
> SfxObjectShell::CloseInternal() (this=0x61b0f30) at sfx2/source/doc/objxtor.cxx:416
> SfxObjectShell::Close() (this=0x61b0f30) at sfx2/source/doc/objxtor.cxx:397
> SotObject::DoClose() (this=0x61b0fd8) at sot/source/base/object.cxx:66
> SotObject::OwnerLock(bool) (this=0x61b0fd8, bLock=false) at sot/source/base/object.cxx:53
> SfxViewFrame::ReleaseObjectShell_Impl() (this=0x5cd4c00) at sfx2/source/view/viewfrm.cxx:1087
> SfxViewFrame::~SfxViewFrame() (this=0x5cd4c00) at sfx2/source/view/viewfrm.cxx:1456
> SfxViewFrame::~SfxViewFrame() (this=0x5cd4c00) at sfx2/source/view/viewfrm.cxx:1450
> SfxViewFrame::Close() (this=0x5cd4c00) at sfx2/source/view/viewfrm.cxx:1112
> SfxFrame::DoClose_Impl() (this=0x20d1720) at sfx2/source/view/frame.cxx:169
> SfxBaseController::dispose() (this=0x7fffbc4d0600) at sfx2/source/view/sfxbasecontroller.cxx:1042
> (anonymous namespace)::Frame::setComponent(com::sun::star::uno::Reference<com::sun::star::awt::XWindow> const&, com::sun::star::uno::Reference<com::sun::star::frame::XController> const&) (this=0x7fff9fcb04b8, xComponentWindow=empty uno::Reference, xController=empty uno::Reference) at framework/source/services/frame.cxx:1538
> (anonymous namespace)::Frame::close(unsigned char) (this=0x7fff9fcb04b8, bDeliverOwnership=0 '\000') at framework/source/services/frame.cxx:1770
> framework::pattern::frame::closeIt(com::sun::star::uno::Reference<com::sun::star::uno::XInterface> const&, bool) (xResource=uno::Reference to ((anonymous namespace)::Frame *) 0x7fff9fcb04e0, bDelegateOwnership=false) at framework/source/inc/pattern/frame.hxx:69
> framework::CloseDispatcher::implts_closeFrame() (this=0x7fffbc4cd848) at framework/source/dispatch/closedispatcher.cxx:489
> framework::CloseDispatcher::impl_asyncCallback(LinkParamNone*) (this=0x7fffbc4cd848) at framework/source/dispatch/closedispatcher.cxx:359
> framework::CloseDispatcher::LinkStubimpl_asyncCallback(void*, LinkParamNone*) (instance=0x7fffbc4cd848, data=0x0) at framework/source/dispatch/closedispatcher.cxx:243
> ...
Comment 25 Commit Notification 2016-03-30 20:47:32 UTC
akash committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=f80de31099be1ba7c6f753187f6703a915151c27

tdf#96896-Fix infinite recursion to prevent Writer crash

It will be available in 5.2.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 26 Commit Notification 2016-04-19 08:15:32 UTC
akash committed a patch related to this issue.
It has been pushed to "libreoffice-5-1":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=0dbd5b7195669225bbe2aafb53aeed577394170e&h=libreoffice-5-1

tdf#96896-Fix infinite recursion to prevent Writer crash

It will be available in 5.1.3.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.