330084 - Crash in [@ nsGenericElement::doReplaceOrInsertBefore]

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Stephen Donner [:stephend] Not actively reading bugmail]

Crash in [@ nsGenericElement::doReplaceOrInsertBefore]

Steps to Reproduce:

1. Load http://www.hollywood.com/tv/listings_search.aspx
2. Type "45617" into the textfield, click the "Go" button
3. On the resulting page, choose "South Bend-Elkhrt - Broadcast"

Expected Results:

Grid appears.

Actual Results:

Crash, see stack, below.

If the stack can be believed, it is:

nsGenericElement::doReplaceOrInsertBefore  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/base/src/nsGenericElement.cpp, line 3015]
nsGenericElement::ReplaceChild  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/base/src/nsGenericElement.cpp, line 2462]
nsHTMLOptionCollection::SetOption  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/html/content/src/nsHTMLSelectElement.cpp, line 2294]
nsHTMLSelectElementSH::SetOption  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/dom/src/base/nsDOMClassInfo.cpp, line 8557]
nsHTMLOptionsCollectionSH::SetProperty  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/dom/src/base/nsDOMClassInfo.cpp, line 9127]
XPC_WN_Helper_SetProperty  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp, line 954]
js_SetProperty  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsobj.c, line 3210]
js_Interpret  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c, line 3658]
js_Execute  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c, line 1497]
JS_EvaluateUCScriptForPrincipals  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsapi.c, line 4134]
nsJSContext::EvaluateString  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/dom/src/base/nsJSEnvironment.cpp, line 1076]
nsScriptLoader::EvaluateScript  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/base/src/nsScriptLoader.cpp, line 761]
nsScriptLoader::ProcessRequest  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/base/src/nsScriptLoader.cpp, line 659]
nsScriptLoader::DoProcessScriptElement  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/base/src/nsScriptLoader.cpp, line 592]
nsScriptLoader::ProcessScriptElement  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/base/src/nsScriptLoader.cpp, line 343]
nsHTMLScriptElement::MaybeProcessScript  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/html/content/src/nsHTMLScriptElement.cpp, line 711]
nsHTMLScriptElement::DoneAddingChildren  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/html/content/src/nsHTMLScriptElement.cpp, line 563]
SinkContext::CloseContainer  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/html/document/src/nsHTMLContentSink.cpp, line 1407]
HTMLContentSink::CloseContainer  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/html/document/src/nsHTMLContentSink.cpp, line 2973]
CNavDTD::CloseContainer  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/parser/htmlparser/src/CNavDTD.cpp, line 2681]
CNavDTD::HandleToken  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/parser/htmlparser/src/CNavDTD.cpp, line 699]
CNavDTD::BuildModel  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/parser/htmlparser/src/CNavDTD.cpp, line 331]
nsParser::BuildModel  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/parser/htmlparser/src/nsParser.cpp, line 1755]
nsParser::ResumeParse  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/parser/htmlparser/src/nsParser.cpp, line 1629]

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase (crashes on load)

With a 2006-03-08 build, it doesn't crash, so it regressed after that.
Probably a regression from bug 330084.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #1)
> With a 2006-03-08 build, it doesn't crash, so it regressed after that.
> Probably a regression from bug 330084.
> 
Er, from what? from this bug? ;)

Comment 3

[Martijn Wargers (dead)]

(In reply to comment #2)
> Er, from what? from this bug? ;)
Er, I meant regression from bug 325730 :) 

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I have a patch for this

Comment 5

[pal-moz]

with QuickPost (Movable Type:Blog Tool)

work : 08 Mar Nightly (non-cairo)
crash : 09/10/11 Mar Nightly (non-cairo)
TB16248259E (09 nightly)
TB16248357H (10 nightly)
TB16248426Q (11 nightly) 

sample:
bookmark <a href="javascript:d=document;w=window;t='';if(d.selection)t=d.selection.createRange().text;else{if(d.getSelection)t=d.getSelection();else{if(w.getSelection)t=w.getSelection()}}void(w.open('http://rouge-web.net/mt/mt.cgi?__mode=view&is_bm=1&_type=entry&bm_show=t%2Cc%2Cac%2Cap%2Ccb%2Ce%2Cm%2Ck%2Cb&link_title='+escape(d.title)+'&link_href='+escape(d.location.href)+'&text='+escape(t),'_blank','scrollbars=yes,width=400,height=880,status=yes,resizable=yes,scrollbars=yes'))">THIS</a>
login (username:Melody,pass:Nelson)
"Select a weblog for this post:">>crash

I think this Probably a regression from Bug 329125 "Remove nsMutationEvent::mTarget r+sr=bz" at (2006-03-09 10:14) or from Bug 325730 "Mutation-event handlers can cause further mutations to the DOM. We need to be more attentive to those. r=bz sr=jst" at (2006-03-08 13:47)

Comment 6

[Stephen Donner [:stephend] Not actively reading bugmail]

No need to comment further when someone has a patch.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix

So this is sort of a two-fold problem. First of all the caller in nsHTMLSelectElement doesn't follow COM rules since it does not hold strong reference to all its arguments.

However, I wonder if we should be able to deal with this inside doReplaceOrInsertBefore. I'd think it's not unlikly that there are other callers like this.

Let me know what you think.

Comment 8

[gabe]

http://www.businessweek.com/technology/content/mar2006/tc20060314_773335.htm

also crashes

Comment 9

[Stephen Donner [:stephend] Not actively reading bugmail]

*** Bug 330620 has been marked as a duplicate of this bug. ***

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 214967 [details] [diff] [review]
Patch to fix

doReplaceOrInsertBefore() isn't strictly a COM method, so whether it follows the rules or not isn't really a big deal to me, as long as all callers do "the right thing". This change does seem good to me though, both parts.

r+sr=jst

Comment 11

[Hermann Schwab]

*** Bug 331011 has been marked as a duplicate of this bug. ***

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Marking fixed. This landed long ago.

Comment 13

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED; this has actually been fixed for a while.

SeaMonkey 1.5a;Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060417 SeaMonkey/1.5a

Comment 14

[Jay Patel [:jay]]

Comment on attachment 214967 [details] [diff] [review]
Patch to fix

a=jay per triage meeting, please get this checked in on the 1.8.0 branch.  Thanks!

Comment 15

[Marcia Knous [:marcia]]

verified fixed on the 1.8.0 branch using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.4) Gecko/20060504 Firefox/1.5.0.4. No crash using the testcase or the original STR. Adding keyword.