The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in mozilla9

Status

()

Core
Layout
--
critical
VERIFIED FIXED
6 years ago
4 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: jesup)

Tracking

({crash, regression, testcase})

Trunk
mozilla9
x86
Windows 7
crash, regression, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox6 wontfix, firefox7+ wontfix, firefox8+ affected, firefox9+ affected, status1.9.2 unaffected)

Details

(Whiteboard: [qa!], crash signature)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

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

Comment 1

6 years ago
Created attachment 556374 [details]
testcase
Randel, can you take this, please?
tracking-firefox8: --- → ?
tracking-firefox9: --- → ?
(Assignee)

Updated

6 years ago
Assignee: nobody → rjesup
Status: NEW → ASSIGNED

Updated

6 years ago
tracking-firefox8: ? → +
tracking-firefox9: ? → +

Updated

6 years ago
tracking-firefox7: --- → ?

Updated

6 years ago
status-firefox7: --- → unaffected
status-firefox8: --- → unaffected
status-firefox9: --- → affected
tracking-firefox7: ? → +
Given the regression range and regressee in comment 0 this bug affects Fx6 onwards. Guessing the "unaffected" settings were just off-by-one selections?
status1.9.2: --- → unaffected
status-firefox6: --- → wontfix
status-firefox7: unaffected → affected
status-firefox8: unaffected → affected
(Assignee)

Comment 4

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

Comment 5

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

Comment 7

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

Comment 9

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

Updated

6 years ago
Attachment #560850 - Attachment is obsolete: true
Attachment #560850 - Flags: feedback?(roc)
(Assignee)

Updated

6 years ago
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+
(Assignee)

Comment 11

6 years ago
committed to inbound as https://hg.mozilla.org/integration/mozilla-inbound/rev/c0f7ee4b3358
Whiteboard: [inbound]
https://hg.mozilla.org/mozilla-central/rev/c0f7ee4b3358
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla9
(Assignee)

Comment 13

6 years ago
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?

Updated

6 years ago
status-firefox7: affected → wontfix

Updated

6 years ago
Attachment #561054 - Flags: approval-mozilla-beta? → approval-mozilla-beta-

Comment 14

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

Comment 16

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

Comment 18

6 years ago
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+
https://hg.mozilla.org/mozilla-central/rev/632255778cab
You need to log in before you can comment on or make changes to this bug.