Closed Bug 682684 Opened 8 years ago Closed 8 years ago

Crash [@ nsTextControlFrame::FinishedInitializer] with mask and generated content and input in mask

Categories

(Core :: Layout, defect, critical)

x86
Windows 7
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla9
Tracking Status
firefox6 --- wontfix
firefox7 + wontfix
firefox8 + affected
firefox9 + affected
status1.9.2 --- unaffected

People

(Reporter: martijn.martijn, Assigned: jesup)

References

Details

(Keywords: crash, regression, testcase, Whiteboard: [qa!])

Crash Data

Attachments

(3 files, 1 obsolete file)

See upcoming testcase, which crashes current trunk build.
This regressed between 2011-05-19 and 2011-06-06, so probably - just like bug 669767 - a regression from bug 655084.

https://crash-stats.mozilla.com/report/index/bp-d946e760-5956-4604-8b67-313582110827
0 	xul.dll 	nsTextControlFrame::FinishedInitializer 	layout/forms/nsTextControlFrame.h:414
1 	xul.dll 	nsTextControlFrame::EditorInitializer::Run 	layout/forms/nsTextControlFrame.h:315
2 	xul.dll 	nsContentUtils::RemoveScriptBlocker 	content/base/src/nsContentUtils.cpp:4379
3 	xul.dll 	nsDocument::EndUpdate 	content/base/src/nsDocument.cpp:3994
4 	xul.dll 	mozAutoDocUpdate::~mozAutoDocUpdate 	content/base/src/mozAutoDocUpdate.h:67
5 	xul.dll 	nsContentSink::NotifyAppend 	content/base/src/nsContentSink.cpp:1351
6 	xul.dll 	nsXMLContentSink::HandleEndElement 	content/xml/document/src/nsXMLContentSink.cpp:1158
7 	xul.dll 	nsXMLContentSink::HandleEndElement 	content/xml/document/src/nsXMLContentSink.cpp:1102
8 	xul.dll 	Driver_HandleEndElement 	parser/htmlparser/src/nsExpatDriver.cpp:106
9 	xul.dll 	doContent 	parser/expat/lib/xmlparse.c:2562
10 	xul.dll 	contentProcessor 	parser/expat/lib/xmlparse.c:2107
11 	xul.dll 	MOZ_XML_ResumeParser 	parser/expat/lib/xmlparse.c:1777
12 	xul.dll 	nsExpatDriver::ParseBuffer 	
13 	xul.dll 	nsExpatDriver::ConsumeToken 	parser/htmlparser/src/nsExpatDriver.cpp:1107
14 	xul.dll 	nsParser::Tokenize 	parser/htmlparser/src/nsParser.cpp:2895
15 	xul.dll 	nsParser::ResumeParse 	parser/htmlparser/src/nsParser.cpp:2186
16 	xul.dll 	nsParser::OnStopRequest 	parser/htmlparser/src/nsParser.cpp:2815
17 	xul.dll 	nsExternalResourceMap::PendingLoad::OnStopRequest 	content/base/src/nsDocument.cpp:1083
18 	xul.dll 	nsBaseChannel::OnStopRequest 	netwerk/base/src/nsBaseChannel.cpp:727
19 	xul.dll 	nsInputStreamPump::OnStateStop 	netwerk/base/src/nsInputStreamPump.cpp:578
20 	xul.dll 	nsInputStreamPump::OnInputStreamReady 	netwerk/base/src/nsInputStreamPump.cpp:403
Attached file testcase
Randel, can you take this, please?
Assignee: nobody → rjesup
Status: NEW → ASSIGNED
Given the regression range and regressee in comment 0 this bug affects Fx6 onwards. Guessing the "unaffected" settings were just off-by-one selections?
This fixes the testcase, and per discussion with ehsan on 8/30 about avoiding
SetSelectionEndPoints() if the frame is gone.  A meta-discussion on the
cause of this sort of bug and the possibility of using static analysis 
occurred by email and led to a bug being filed on that.

So far I've left the NS_ASSERTION in, but probably if we take this patch we
should remove it (though it only affects debug builds of course).

Here's the current output:

###!!! ASSERTION: Frame destroyed even though we had a scriptblocker: 'mFrame', file ../../../layout/forms/nsTextControlFrame.h, line 314
WARNING: NS_ENSURE_TRUE(window) failed: file ../../../../extensions/spellcheck/src/mozInlineSpellWordUtil.cpp, line 109

I haven't investigated if the warning is to be expected or if we need to
worry about it yet.
Comment on attachment 560850 [details] [diff] [review]
Patch to handle mFrame revocation

roc - Feel free to review instead of giving feedback if you think this is ready and the warning is inconsequential.
Attachment #560850 - Flags: feedback?(roc)
Comment on attachment 560850 [details] [diff] [review]
Patch to handle mFrame revocation

Review of attachment 560850 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/forms/nsTextControlFrame.h
@@ +311,1 @@
>          mFrame->EnsureEditorInitialized();

If this can cause the frame to be destroyed and mFrame set to null, we shouldn't be asserting it's non-null below...
roc: you probably missed the comment before the patch:

>So far I've left the NS_ASSERTION in, but probably if we take this patch we
>should remove it (though it only affects debug builds of course).

Are you good with it otherwise?
The Beta triage team asked about volume of this crash in crash-stats. From what I can see in crash stats there are no crashes in this stack in the last 4 weeks.
Updated patch to remove assertion and slightly improve code clarity; ready
for review.  It's good to know there have been virtually no hits on this
bug in the wild, but this should close the window in a very safe manner (I
can't see this patch introducing any new bugs, though in theory it might
expose an existing bug elsewhere).
Attachment #560850 - Attachment is obsolete: true
Attachment #560850 - Flags: feedback?(roc)
Attachment #561054 - Flags: review?(roc)
Comment on attachment 561054 [details] [diff] [review]
Patch to handle mFrame revocation, updated

Review of attachment 561054 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/forms/nsTextControlFrame.cpp
@@ +385,5 @@
>    mUseEditor = PR_TRUE;
>  
>    // Set the selection to the beginning of the text field.
> +  if (weakFrame.IsAlive())
> +    SetSelectionEndPoints(0, 0);

{}
Attachment #561054 - Flags: review?(roc) → review+
https://hg.mozilla.org/mozilla-central/rev/c0f7ee4b3358
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla9
Comment on attachment 561054 [details] [diff] [review]
Patch to handle mFrame revocation, updated

Requesting to land in Aurora and in Beta (if it's not too late; it may well be)

Note: patch as committed to inbound has the {} requested by roc
Attachment #561054 - Flags: approval-mozilla-beta?
Attachment #561054 - Flags: approval-mozilla-aurora?
Attachment #561054 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment on attachment 561054 [details] [diff] [review]
Patch to handle mFrame revocation, updated

we're a little too late in Aurora (last triage) to take crash fix changes that don't come with a big bang for the buck (topcrashes).
Attachment #561054 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
qa- unless Martijn wants to verify this bug fix
Whiteboard: [qa-]
Verified fixed, using nightly build: 9.0a1 (2011-09-20).
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
Whiteboard: [qa-] → [qa+]
Martijn, can you verify on Firefox 8 as well? If it's okay, add the verified-aurora keyword and change the whiteboard tag to [qa!]

Thanks
It's still crashing in Firefox 8, I checked with current nightly firefox 8 build.
Asa didn't approve the patch for Aurora, see comment 14. Also, the target milestone is set to mozilla9.
(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #18)
> It's still crashing in Firefox 8, I checked with current nightly firefox 8
> build.
> Asa didn't approve the patch for Aurora, see comment 14. Also, the target
> milestone is set to mozilla9.

Seen, thanks. Setting whiteboard tag to [qa!] as we won't need to verify this elsewhere.
Whiteboard: [qa+] → [qa!]
Crash test:
https://hg.mozilla.org/integration/mozilla-inbound/rev/632255778cab
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.