Last Comment Bug 626014 - Crash when hiding Menubar with URL and search box [@ nsTextControlFrame::SetSelectionInternal(nsIDOMNode*, int, nsIDOMNode*, int) ]
: Crash when hiding Menubar with URL and search box [@ nsTextControlFrame::SetS...
Status: RESOLVED FIXED
[softblocker]
: crash, regression
Product: Core
Classification: Components
Component: Selection (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla2.0b10
Assigned To: Mats Palmgren (:mats)
:
Mentors:
: 626288 (view as bug list)
Depends on:
Blocks: 602331 625771
  Show dependency treegraph
 
Reported: 2011-01-14 22:52 PST by Biju
Modified: 2011-06-13 10:01 PDT (History)
9 users (show)
mats: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
final+


Attachments
nomenu_crash.png (29.53 KB, image/png)
2011-01-14 22:52 PST, Biju
no flags Details
stack leading up to frame destruction (12.19 KB, text/plain)
2011-01-16 18:16 PST, Mats Palmgren (:mats)
no flags Details
Patch v1 (1.24 KB, patch)
2011-01-16 18:32 PST, Mats Palmgren (:mats)
no flags Details | Diff | Review
stack (6.32 KB, text/plain)
2011-01-16 20:39 PST, Mats Palmgren (:mats)
no flags Details
Patch v2 (2.27 KB, patch)
2011-01-16 21:03 PST, Mats Palmgren (:mats)
ehsan: review-
Details | Diff | Review
Patch v3 (2.16 KB, patch)
2011-01-17 18:44 PST, Mats Palmgren (:mats)
ehsan: review+
Details | Diff | Review

Description Biju 2011-01-14 22:52:09 PST
Created attachment 504072 [details]
nomenu_crash.png

Mozilla/5.0 (Windows NT 6.0; rv:2.0b10pre) Gecko/20110114 Firefox/4.0b10pre
http://crash-stats.mozilla.com/report/index/bp-b5c26952-00b0-452a-a143-b59a82110114
http://crash-stats.mozilla.com/report/index/bp-1fb5be51-1765-4cde-b4bf-2ab162110114
http://crash-stats.mozilla.com/report/index/bp-9dd28713-3e08-4f54-9a98-aed002110114


I have URL and search bar on Menubar
Now when I hide Menubar it crashes
See my layout on nomenu_crash.png
If I reset layout and try to hide Menubar there is no problem.
Comment 1 Matthias Versen [:Matti] 2011-01-14 23:45:09 PST
probably a dupe of bug 625771

0 	xul.dll 	nsTextControlFrame::SetSelectionInternal 	layout/forms/nsTextControlFrame.cpp:859
1 	xul.dll 	nsTextControlFrame::SetSelectionEndPoints 	layout/forms/nsTextControlFrame.cpp:942
2 	xul.dll 	nsTextControlFrame::SetSelectionRange 	layout/forms/nsTextControlFrame.cpp:958
3 	xul.dll 	RestoreSelectionState::Run 	content/html/content/src/nsTextEditorState.cpp:95
4 	xul.dll 	nsContentUtils::AddScriptRunner 	
5 	xul.dll 	PrepareEditorEvent::Run 	content/html/content/src/nsTextEditorState.cpp:1040
6 	xul.dll 	nsContentUtils::RemoveScriptBlocker 	content/base/src/nsContentUtils.cpp:4722
7 	xul.dll 	nsDocument::EndUpdate 	content/base/src/nsDocument.cpp:3969
8 	xul.dll 	nsXULDocument::EndUpdate 	content/xul/document/src/nsXULDocument.cpp:3330
9 	xul.dll 	mozAutoDocUpdate::~mozAutoDocUpdate 	content/base/src/mozAutoDocUpdate.h:66
10 	xul.dll 	nsGenericElement::SetAttrAndNotify 	content/base/src/nsGenericElement.cpp:4725
11 	xul.dll 	nsGenericElement::SetAttr 	content/base/src/nsGenericElement.cpp:4623
12 	xul.dll 	nsGenericElement::SetAttribute 	content/base/src/nsGenericElement.cpp:2386
13 	xul.dll 	nsIDOMElement_SetAttribute 	obj-firefox/js/src/xpconnect/src/dom_quickstubs.cpp:4853
...
Comment 2 Alice0775 White 2011-01-15 00:10:37 PST
Confirmed on 
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b10pre) Gecko/20110114 Firefox/4.0b10pre ID:20110114030359

Browser crashes with crash reports
bp-ba3664d7-c94d-4ea4-9135-8e28a2110115

[str]
1. Start Minefield with clean profile
2. Alt > View > Toolbars > Check Menu Bar(disable auto hide)
3. Alt > View > Toolbars > Customize…
4. Drag and drop Location Bar to Menu Bar
5. Alt > View > Toolbars > Un-check Menu Bar(enable auto hide)

[actual]
  Browser crashes with crash reports

Regression window(cached hourly):
Works:
http://hg.mozilla.org/mozilla-central/rev/2894886924fc
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b10pre) Gecko/20110113 Firefox/4.0b10pre ID:20110113161101
Fails:
http://hg.mozilla.org/mozilla-central/rev/5efe5f98ac13
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b10pre) Gecko/20110113 Firefox/4.0b10pre ID:20110113162644
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2894886924fc&tochange=5efe5f98ac13
Comment 3 Mats Palmgren (:mats) 2011-01-16 18:16:37 PST
Created attachment 504339 [details]
stack leading up to frame destruction
Comment 4 Mats Palmgren (:mats) 2011-01-16 18:32:53 PST
Created attachment 504340 [details] [diff] [review]
Patch v1

Bug 602331 added a flush in nsTypedSelection::AddRange so consumers
needs to check for frame destruction etc.  This should fix the crash in
nsTextControlFrame::SetSelectionInternal.

We used to flush at an even lower level, in nsTypedSelection::selectFrames,
but bug 527306 removed that 2009-11-15.  I'm checking the other call sites...
Comment 5 Mats Palmgren (:mats) 2011-01-16 18:40:10 PST
*** Bug 626288 has been marked as a duplicate of this bug. ***
Comment 6 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-01-16 19:31:43 PST
How does this patch fix the crash? We're holding a strong reference to selCon so we couldn't crash dereferencing it.
Comment 7 Mats Palmgren (:mats) 2011-01-16 20:07:55 PST
'selCon' in SetSelectionInternal is just a raw pointer.
It's owned by nsTextEditorState::mSelCon which is owned by nsHTMLInputElement,
but destroying the frame calls nsTextEditorState::UnbindFromFrame
which sets mSelCon to null.
http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsTextEditorState.cpp#1451

So I think we could fix it by just making 'selCon' a strong ref locally
in SetSelectionInternal and trusting its ScrollSelectionIntoView to do
the right thing when the frame is gone (which it looks like it's doing)...
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-01-16 20:16:25 PST
Let's do that!
Comment 9 Mats Palmgren (:mats) 2011-01-16 20:39:58 PST
Created attachment 504352 [details]
stack

FTR, I did verify the above in GDB just to be sure.

After making 'selCon' strong, the refcount is 1 in ScrollSelectionIntoView.
It does an early return NS_ERROR_FAILURE when 'mFrameSelection' is null.
mFrameSelection is set to null in nsTextInputSelectionImpl::SetScrollableFrame
(called from UnbindFromFrame, see link in comment above).

So this all looks good, except maybe that SetSelectionInternal returns
what ScrollSelectionIntoView returns (NS_ERROR_FAILURE).
Comment 10 Mats Palmgren (:mats) 2011-01-16 21:03:04 PST
Created attachment 504355 [details] [diff] [review]
Patch v2

Additionally, adding a frame destruction check to SetFormProperty.
SetFormProperty
  SelectAllOrCollapseToEndOfText
    SetSelectionInternal
Comment 11 :Ehsan Akhgari (busy, don't ask for review please) 2011-01-17 12:50:12 PST
Comment on attachment 504355 [details] [diff] [review]
Patch v2

I don't like this patch because of two reasons:

1. We're introducing failures stemming from SetSelectionInternal which is a web visible API, which means that our APIs might start to fail in cases which used to not fail before.

2. We're relying on ScrollSelectionIntoView for a detached selection controller object to do the right thing, always.

I think a better fix would be to just regrab the selection controller after the AddRange call.

BTW, I think we can use the testcase in bug 626288 as a crashtest here.  It would be useful to run the testcase under valgrind to make sure that we're catching all of the loose ends here.
Comment 12 Mats Palmgren (:mats) 2011-01-17 18:44:50 PST
Created attachment 504603 [details] [diff] [review]
Patch v3

Agreed, this is a better fix.

I'll land the test in bug 626288 together with the fix (not included in the
patch to protect our nightly testers).

The test passed valgrind testing on Linux64 (all crashtests passed btw).
Comment 13 :Ehsan Akhgari (busy, don't ask for review please) 2011-01-17 18:47:55 PST
Comment on attachment 504603 [details] [diff] [review]
Patch v3

This is great!  But please add a comment explaining why we're grabbing the selection controller again.

Thanks!
Comment 14 :Ehsan Akhgari (busy, don't ask for review please) 2011-01-17 18:48:33 PST
(In reply to comment #12)
> I'll land the test in bug 626288 together with the fix (not included in the
> patch to protect our nightly testers).

Good choice.

> The test passed valgrind testing on Linux64 (all crashtests passed btw).

Thanks for testing it under valgrind.  This makes me feel better!  :-)
Comment 15 Mats Palmgren (:mats) 2011-01-18 13:32:28 PST
Landed with the additional code comment and crashtest:
http://hg.mozilla.org/mozilla-central/rev/65844bb7d0c5
http://hg.mozilla.org/mozilla-central/rev/8b5fb8ac2125

Note You need to log in before you can comment on or make changes to this bug.