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

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Martijn Wargers (dead)]

Attachment: mask file needed for testcase

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

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[(no longer active)]

Randel, can you take this, please?

Comment 3

[Daniel Veditz [:dveditz]]

Given the regression range and regressee in comment 0 this bug affects Fx6 onwards. Guessing the "unaffected" settings were just off-by-one selections?

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Patch to handle mFrame revocation

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 5

[Randell Jesup [:jesup] (needinfo me)]

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.

Comment 6

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

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...

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

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?

Comment 8

[Marcia Knous [:marcia]]

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.

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Patch to handle mFrame revocation, updated

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).

Comment 10

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

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);

{}

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

committed to inbound as https://hg.mozilla.org/integration/mozilla-inbound/rev/c0f7ee4b3358

Comment 12

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/c0f7ee4b3358

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 14

[Asa Dotzler [:asa]]

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).

Comment 15

[u279076]

qa- unless Martijn wants to verify this bug fix

Comment 16

[Martijn Wargers (dead)]

Verified fixed, using nightly build: 9.0a1 (2011-09-20).

Comment 17

[u279076]

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

Comment 18

[Martijn Wargers (dead)]

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.

Comment 19

[u279076]

(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.

Comment 20

[Mats Palmgren (inactive)]

Crash test:
https://hg.mozilla.org/integration/mozilla-inbound/rev/632255778cab

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/632255778cab