Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in mozilla2.0b10

Status

()

Core
Selection
--
critical
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Biju, Assigned: Mats Palmgren (vacation - back in August))

Tracking

({crash, regression})

Trunk
mozilla2.0b10
crash, regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 final+)

Details

(Whiteboard: [softblocker], crash signature)

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

7 years ago
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.
(Reporter)

Updated

7 years ago
Severity: normal → critical
(Reporter)

Updated

7 years ago
Blocks: 572160
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
...
Component: Menus → Editor
Product: Firefox → Core
QA Contact: menus → editor
Summary: Crash when hiding Menubar with URL and search box → Crash when hiding Menubar with URL and search box [@ nsTextControlFrame::SetSelectionInternal(nsIDOMNode*, int, nsIDOMNode*, int) ]
Version: unspecified → Trunk

Comment 2

7 years ago
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
Blocks: 602331
No longer blocks: 572160
Assignee: nobody → matspal
blocking2.0: --- → ?
Keywords: regression
Created attachment 504339 [details]
stack leading up to frame destruction
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...
Attachment #504340 - Flags: review?(ehsan)
Duplicate of this bug: 626288
Component: Editor → Selection
OS: Windows Vista → All
QA Contact: editor → selection
Hardware: x86 → All
Blocks: 625771
How does this patch fix the crash? We're holding a strong reference to selCon so we couldn't crash dereferencing it.
blocking2.0: ? → final+
Whiteboard: [hardblocker]
Whiteboard: [hardblocker] → [softblocker]
'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)...
Let's do that!
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).
Created attachment 504355 [details] [diff] [review]
Patch v2

Additionally, adding a frame destruction check to SetFormProperty.
SetFormProperty
  SelectAllOrCollapseToEndOfText
    SetSelectionInternal
Attachment #504340 - Attachment is obsolete: true
Attachment #504355 - Flags: review?(ehsan)
Attachment #504340 - Flags: review?(ehsan)
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.
Attachment #504355 - Flags: review?(ehsan) → review-
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).
Attachment #504603 - Flags: review?(ehsan)
Attachment #504355 - Attachment is obsolete: true
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!
Attachment #504603 - Flags: review?(ehsan) → review+
(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!  :-)
Landed with the additional code comment and crashtest:
http://hg.mozilla.org/mozilla-central/rev/65844bb7d0c5
http://hg.mozilla.org/mozilla-central/rev/8b5fb8ac2125
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b10
Crash Signature: [@ nsTextControlFrame::SetSelectionInternal(nsIDOMNode*, int, nsIDOMNode*, int) ]
You need to log in before you can comment on or make changes to this bug.