Closed Bug 330084 Opened 15 years ago Closed 15 years ago

Crash in [@ nsGenericElement::doReplaceOrInsertBefore]

Categories

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

x86
Windows XP
defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: stephend, Assigned: sicking)

References

Details

(5 keywords)

Crash Data

Attachments

(2 files)

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]
With a 2006-03-08 build, it doesn't crash, so it regressed after that.
Probably a regression from bug 330084.
Keywords: testcase
(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? ;)
(In reply to comment #2)
> Er, from what? from this bug? ;)
Er, I meant regression from bug 325730 :) 

I have a patch for this
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)
No need to comment further when someone has a patch.
Attached patch Patch to fixSplinter Review
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.
Attachment #214967 - Flags: superreview?(jst)
Attachment #214967 - Flags: review?(jst)
Assignee: general → bugmail
*** Bug 330620 has been marked as a duplicate of this bug. ***
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
Attachment #214967 - Flags: superreview?(jst)
Attachment #214967 - Flags: superreview+
Attachment #214967 - Flags: review?(jst)
Attachment #214967 - Flags: review+
*** Bug 331011 has been marked as a duplicate of this bug. ***
Flags: blocking1.9a1+
Marking fixed. This landed long ago.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → VERIFIED
Attachment #214967 - Flags: approval-branch-1.8.1?(jst)
Flags: blocking1.8.0.3?
Attachment #214967 - Flags: approval-branch-1.8.1?(jst) → approval-branch-1.8.1+
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!
Attachment #214967 - Flags: approval1.8.0.3? → approval1.8.0.3+
Flags: blocking1.8.0.3? → blocking1.8.0.3+
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.
Crash Signature: [@ nsGenericElement::doReplaceOrInsertBefore]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.