626014 - Crash when hiding Menubar with URL and search box [@ nsTextControlFrame::SetSelectionInternal(nsIDOMNode*, int, nsIDOMNode*, int) ]

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Biju]

Attachment: 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]]

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]

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 (inactive)]

Attachment: stack leading up to frame destruction

Comment 4

[Mats Palmgren (inactive)]

Attachment: 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 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 (inactive)]

'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) (email my personal email if necessary)]

Let's do that!

Comment 9

[Mats Palmgren (inactive)]

Attachment: 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 (inactive)]

Attachment: Patch v2

Additionally, adding a frame destruction check to SetFormProperty.
SetFormProperty
  SelectAllOrCollapseToEndOfText
    SetSelectionInternal

Comment 11

[(no longer active)]

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 (inactive)]

Attachment: 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

[(no longer active)]

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

[(no longer active)]

(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 (inactive)]

Landed with the additional code comment and crashtest:
http://hg.mozilla.org/mozilla-central/rev/65844bb7d0c5
http://hg.mozilla.org/mozilla-central/rev/8b5fb8ac2125