Last Comment Bug 682684 - Crash [@ nsTextControlFrame::FinishedInitializer] with mask and generated content and input in mask
: Crash [@ nsTextControlFrame::FinishedInitializer] with mask and generated con...
Status: VERIFIED FIXED
[qa!]
: crash, regression, testcase
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: x86 Windows 7
: -- critical (vote)
: mozilla9
Assigned To: Randell Jesup [:jesup]
:
Mentors:
Depends on:
Blocks: 655084
  Show dependency treegraph
 
Reported: 2011-08-28 01:58 PDT by Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
Modified: 2013-05-19 17:48 PDT (History)
8 users (show)
mats: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
+
wontfix
+
affected
+
affected
unaffected


Attachments
mask file needed for testcase (146 bytes, text/xml)
2011-08-28 01:58 PDT, Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
no flags Details
testcase (147 bytes, application/xhtml+xml)
2011-08-28 02:00 PDT, Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
no flags Details
Patch to handle mFrame revocation (2.12 KB, patch)
2011-09-18 22:12 PDT, Randell Jesup [:jesup]
no flags Details | Diff | Review
Patch to handle mFrame revocation, updated (2.17 KB, patch)
2011-09-19 15:56 PDT, Randell Jesup [:jesup]
roc: review+
asa: approval‑mozilla‑aurora-
christian: approval‑mozilla‑beta-
Details | Diff | Review

Description Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2011-08-28 01:58:45 PDT
Created attachment 556373 [details]
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 [:mwargers] (gone per 2016-05-31 :-( ) 2011-08-28 02:00:26 PDT
Created attachment 556374 [details]
testcase
Comment 2 :Ehsan Akhgari (busy, don't ask for review please) 2011-08-30 12:51:11 PDT
Randel, can you take this, please?
Comment 3 Daniel Veditz [:dveditz] 2011-09-18 07:27:33 PDT
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] 2011-09-18 22:12:28 PDT
Created attachment 560850 [details] [diff] [review]
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] 2011-09-18 22:15:06 PDT
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) (Exited; email my personal email if necessary) 2011-09-18 22:35:18 PDT
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] 2011-09-18 23:08:04 PDT
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 - use ni] 2011-09-19 15:28:35 PDT
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] 2011-09-19 15:56:49 PDT
Created attachment 561054 [details] [diff] [review]
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) (Exited; email my personal email if necessary) 2011-09-19 16:18:50 PDT
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] 2011-09-19 21:34:48 PDT
committed to inbound as https://hg.mozilla.org/integration/mozilla-inbound/rev/c0f7ee4b3358
Comment 12 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-20 07:50:25 PDT
https://hg.mozilla.org/mozilla-central/rev/c0f7ee4b3358
Comment 13 Randell Jesup [:jesup] 2011-09-20 07:55:47 PDT
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] 2011-09-22 14:23:05 PDT
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 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-09-22 16:59:21 PDT
qa- unless Martijn wants to verify this bug fix
Comment 16 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2011-09-22 21:34:10 PDT
Verified fixed, using nightly build: 9.0a1 (2011-09-20).
Comment 17 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-09-22 21:37:58 PDT
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 [:mwargers] (gone per 2016-05-31 :-( ) 2011-09-22 21:57:49 PDT
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 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-09-22 22:00:21 PDT
(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 (:mats) 2013-05-19 09:35:52 PDT
Crash test:
https://hg.mozilla.org/integration/mozilla-inbound/rev/632255778cab
Comment 21 Phil Ringnalda (:philor) 2013-05-19 17:48:54 PDT
https://hg.mozilla.org/mozilla-central/rev/632255778cab

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