Last Comment Bug 315189 - Loading this url crashes [@ nsHTMLDocument::MatchLinks] Camino and Firefox
: Loading this url crashes [@ nsHTMLDocument::MatchLinks] Camino and Firefox
: crash, fixed1.8, regression, testcase
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: P1 critical with 1 vote (vote)
: mozilla1.8final
Assigned To: Boris Zbarsky [:bz]
Depends on:
Blocks: 312695 315999
  Show dependency treegraph
Reported: 2005-11-05 03:47 PST by Isaac
Modified: 2011-06-09 14:58 PDT (History)
16 users (show)
asa: blocking1.8rc2-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

oops, doesn't include all the js (62.74 KB, text/html)
2005-11-05 03:49 PST, Isaac
no flags Details
warning, this may crash your browser! (76.56 KB, text/html)
2005-11-05 03:56 PST, Isaac
no flags Details
Not minimal testcase (8.66 KB, text/html)
2005-11-05 05:31 PST, Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
no flags Details
Minimal testcase that crashes (350 bytes, text/html)
2005-11-05 06:45 PST, Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
no flags Details
Branch-safe patch (4.08 KB, patch)
2005-11-06 11:57 PST, Boris Zbarsky [:bz]
jst: review+
jst: superreview+
mscott: approval1.8rc2+
Details | Diff | Review

Description Isaac 2005-11-05 03:47:51 PST
User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8) Gecko/20051101 Camino/1.0+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8) Gecko/20051101 Camino/1.0+

Not much to add.  Will attach a test case to show the crashing...

Reproducible: Always

Steps to Reproduce:
1. Load the url

Actual Results:  

Expected Results:  
Not crashing?  =)
Comment 1 Isaac 2005-11-05 03:49:08 PST
Created attachment 201919 [details]
oops, doesn't include all the js
Comment 2 Isaac 2005-11-05 03:56:05 PST
Created attachment 201921 [details]
warning, this may crash your browser!

Oops, left out one of the included js files.  Now they are all in the <head> and the attached html will crash my browser.
Comment 3 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2005-11-05 05:31:33 PST
Created attachment 201925 [details]
Not minimal testcase

Confirmed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051104 Firefox/1.6a1

Talkback ID: TB11476502X

nsHTMLDocument::MatchLinks  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/content/html/document/src/nsHTMLDocument.cpp, line 1692]
nsContentList::PopulateWith  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/content/base/src/nsContentList.cpp, line 793]
nsContentList::PopulateWith  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/content/base/src/nsContentList.cpp, line 793]
nsContentList::PopulateWith  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/content/base/src/nsContentList.cpp, line 793]
nsContentList::PopulateWith  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/content/base/src/nsContentList.cpp, line 793]
nsContentList::PopulateWith  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/content/base/src/nsContentList.cpp, line 793]
nsContentList::ContentAppended  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/content/base/src/nsContentList.cpp, line 644]
nsDocument::ContentAppended  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/content/base/src/nsDocument.cpp, line 2304]
nsHTMLDocument::ContentAppended  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/content/html/document/src/nsHTMLDocument.cpp, line 1095]
nsFragmentObserver::Notify  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/content/base/src/nsGenericElement.cpp, line 3239]
CSSLoaderImpl::LoadInlineStyle  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/style/nsCSSLoader.cpp, line 1608]
nsStyleLinkElement::UpdateStyleSheet  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/content/base/src/nsStyleLinkElement.cpp, line 307]
nsHTMLStyleElement::BindToTree  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/content/html/content/src/nsHTMLStyleElement.cpp, line 223]
nsGenericHTMLElement::BindToTree  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/content/html/content/src/nsGenericHTMLElement.cpp, line 1313]
nsGenericHTMLElement::BindToTree  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/content/html/content/src/nsGenericHTMLElement.cpp, line 1313]
nsGenericHTMLElement::BindToTree  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/content/html/content/src/nsGenericHTMLElement.cpp, line 1313]
nsGenericHTMLElement::BindToTree  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/content/html/content/src/nsGenericHTMLElement.cpp, line 1313]
nsGenericHTMLElement::BindToTree  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/content/html/content/src/nsGenericHTMLElement.cpp, line 1313]
nsGenericElement::InsertChildAt  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/content/base/src/nsGenericElement.cpp, line 2716]
nsGenericElement::doInsertBefore  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/content/base/src/nsGenericElement.cpp, line 3394]
nsGenericElement::InsertBefore  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/content/base/src/nsGenericElement.cpp, line 3023]
nsGenericHTMLElementTearoff::SetInnerHTML  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/content/html/content/src/nsGenericHTMLElement.cpp, line 214]
XPCWrappedNative::CallMethod  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp, line 2139]
XPC_WN_GetterSetter  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp, line 1468]
js_Invoke  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/js/src/jsinterp.c, line 1177]
js_InternalInvoke  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/js/src/jsinterp.c, line 1274]
js_InternalGetOrSet  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/js/src/jsinterp.c, line 1333]
js_SetProperty  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/js/src/jsobj.c, line 3024]
js_Interpret  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/js/src/jsinterp.c, line 3360]
js_Invoke  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/js/src/jsinterp.c, line 1197]
js_Interpret  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/js/src/jsinterp.c, line 3523]
js_Execute  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/js/src/jsinterp.c, line 1424]
JS_EvaluateUCScriptForPrincipals  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/js/src/jsapi.c, line 4102]
nsJSContext::EvaluateString  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/dom/src/base/nsJSEnvironment.cpp, line 1072]
nsScriptLoader::EvaluateScript  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/content/base/src/nsScriptLoader.cpp, line 741]
nsScriptLoader::ProcessRequest  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/content/base/src/nsScriptLoader.cpp, line 639]
nsScriptLoader::ProcessScriptElement  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/content/base/src/nsScriptLoader.cpp, line 574]
nsHTMLScriptElement::MaybeProcessScript  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/content/html/content/src/nsHTMLScriptElement.cpp, line 688]
nsHTMLScriptElement::DoneAddingChildren  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/content/html/content/src/nsHTMLScriptElement.cpp, line 558]
SinkContext::CloseContainer  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/content/html/document/src/nsHTMLContentSink.cpp, line 1398]
HTMLContentSink::CloseContainer  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/content/html/document/src/nsHTMLContentSink.cpp, line 2925]
CNavDTD::CloseContainer  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/parser/htmlparser/src/CNavDTD.cpp, line 2886]
CNavDTD::HandleToken  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/parser/htmlparser/src/CNavDTD.cpp, line 838]
CNavDTD::BuildModel  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/parser/htmlparser/src/CNavDTD.cpp, line 459]
nsParser::BuildModel  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/parser/htmlparser/src/nsParser.cpp, line 2010]
Comment 4 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2005-11-05 06:45:44 PST
Created attachment 201928 [details]
Minimal testcase that crashes
Comment 5 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2005-11-05 07:08:47 PST
Doesn't crash 2005-10-18 trunk build, does crash 2005-10-19 trunk build.
I've also tested that it crashes 2005-10-23 branch build.
Somehow a regression from fixing bug 312847?
Comment 6 Sébastien Auger 2005-11-05 12:35:49 PST
I am crashing with both recent branch and trunk builds but not crashing with the url you posted. 
Comment 7 Mike Schroepfer 2005-11-05 18:02:04 PST
Bz - is this something you can take a look at?
Comment 8 Boris Zbarsky [:bz] 2005-11-06 09:49:15 PST
Ugh.  So what's going on here is this:

1)  We set innerHTML to "<div><style type=text/css></style><a href=#></a>"
2)  This calls BindToTree on the <div>, which recurses over the kids
3)  When the <style> node is bound, it creates a new stylesheet and inserts it
    into the document.
4)  The BeginUpdate() call from this triggers the fragment observer to notify on
    the <div>'s append (see bug 312695).
5)  The document.links list ends up looking at the <a> node, which is already in
    the DOM but hasn't been bound yet(!)
6)  We crash when doing |aContent->GetCurrentDoc()->GetDefaultNamespaceID();| on
    the <a> node.

Now I can work around this crash by not notifying here for STYLE updates, but that would effectively reintroduce bug 312695.  Also, the crash could still be triggered by using a <script> instead of <style>.

Alternately, we could also add a null-check to MatchLinks, which would fix this crash but the rendering would still be broken -- the content after the <style> in that innerHTML would not show up (since we'd try to construct the frame for the <a> while it has no document, so it would never get a frame).

Another option would be to remove this FragmentObserver code altogether.  That would reintroduce bug 312695, but perhaps that's the best we can do under the circumstances...  I don't really think we want to take the patch for bug 314776 (which fixes bug 312695 the "right way" in my opinion) on the branch.  Given the testcases in bug 314776 this might be the way to go.

Finally, I could try to change the FragmentObserver to only notify on nodes that have been "fully" inserted into the DOM (that is, nodes that have already had BindToTree called on the whole subtree rooted at them).  That would reintroduce bug 312695 in some cases (but as you can see from bug 314776 it's present in some cases anyway), but not in the testcase actually attached to bug 314776.  Further, it would mean that inserting a fragment has exactly the issues that inserting a single node does, no more and no less (since the doubling issues would be restricted to the subtree rooted at the node whose descendant is messing with document state from inside BindToTree).

Thoughts?  The more I think about it, the more I like the last option...

For 1.9, of course, we should make BindToTree impls not mess with document state.
Comment 9 Asa Dotzler [:asa] 2005-11-06 10:33:27 PST
unless there is a security implication here that I'm not seeing, or this is a topcrash (which I can't find) then this isn't going to block us.
Comment 10 Boris Zbarsky [:bz] 2005-11-06 10:38:16 PST
This is a null pointer dereference, so not exploitable.

The question, I guess, is what, if anything, we do about this bug for 1.8.x.  For 1.9 I think we should do what I said in comment 8.
Comment 11 Boris Zbarsky [:bz] 2005-11-06 11:57:05 PST
Created attachment 202018 [details] [diff] [review]
Branch-safe patch
Comment 12 Worcester12345 2005-11-08 06:40:18 PST
Version says "Trunk", but I am also getting it with branch:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051107 Firefox/1.5 ID:2005110708
Comment 13 Scott MacGregor 2005-11-11 14:09:36 PST
We're going to do a respin on the branch to pick this change up + the plugins crash. I'll ping jst for the review. bz, lemme know if you need me to land this on the branch for you today pending review comments.
Comment 14 Scott MacGregor 2005-11-11 14:49:12 PST
we have people at Yahoo! standing by to help us verify this fix and to confirm that it fixes the issue completely for them once we have new branch builds with the fix.
Comment 15 Johnny Stenback (:jst, 2005-11-11 14:50:25 PST
Comment on attachment 202018 [details] [diff] [review]
Branch-safe patch

Comment 16 Boris Zbarsky [:bz] 2005-11-11 15:06:11 PST
Fixed on trunk
Comment 17 Boris Zbarsky [:bz] 2005-11-11 15:07:42 PST
Comment on attachment 202018 [details] [diff] [review]
Branch-safe patch

Requesting branch approval...  I think we've gone over the risk/benefit of this already.
Comment 18 Scott MacGregor 2005-11-11 15:07:47 PST
Comment on attachment 202018 [details] [diff] [review]
Branch-safe patch

we'll kick off the respins once this is in so we can get everyone looking at it.
Comment 19 Scott MacGregor 2005-11-11 15:11:23 PST
Comment on attachment 202018 [details] [diff] [review]
Branch-safe patch

my approval didn't stick.
Comment 20 Scott MacGregor 2005-11-11 15:17:59 PST
Boris, can you provide some feedback on areas QA should try to focus on that would be effected by this patch? So we can get some focused testing in addition to verifying that this fixes Yahoo! and the test case. Thanks.
Comment 21 Scott MacGregor 2005-11-11 16:02:51 PST
I landed this on the 1.8 branch for bz. We'll kick off the respins in a few minutes. 

Comment 22 Boris Zbarsky [:bz] 2005-11-11 16:26:44 PST
This patch affects two things:

1)  Setting of innerHTML.
2)  Appending of document fragment nodes

(since the former is implemented in terms of the latter).

I think the best thing to test is setting innerHTML to all sorts of stuff and seeing how it goes.  Especially with scripts and stylesheets in the "stuff", but possibly also with <object> nodes.  Especially with <object> nodes if the adblock  extension is installed; that triggers some fun codepaths...

I've verified that this patch doesn't regress any testcases in the bugs that are linked to this one via dependency chains, of course, but perhaps we should also look at old innerHTML bugs...
Comment 23 Peter van der Woude [:Peter6] 2005-11-11 18:16:44 PST
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8) Gecko/20051111 Firefox/1.5 ID:2005111116

verified, no crash
Comment 24 Stephen Donner [:stephend] - PTO; back on 5/28 2005-11-11 20:12:20 PST
Verified FIXED using SeaMonkey 1.5a;Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051116 Mozilla/1.0.

Trunk, no crash.
Comment 25 Sébastien Auger 2005-11-16 10:34:04 PST
fixed for all testcases.

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