The default bug view has changed. See this FAQ.

Loading this url crashes [@ nsHTMLDocument::MatchLinks] Camino and Firefox

VERIFIED FIXED in mozilla1.8final

Status

()

Core
DOM: Core & HTML
P1
critical
VERIFIED FIXED
12 years ago
6 years ago

People

(Reporter: Isaac, Assigned: bz)

Tracking

(4 keywords)

Trunk
mozilla1.8final
crash, fixed1.8, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8rc2 -

Firefox Tracking Flags

(Not tracked)

Details

(crash signature, URL)

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

12 years ago
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:  
Crashes

Expected Results:  
Not crashing?  =)
(Reporter)

Comment 1

12 years ago
Created attachment 201919 [details]
oops, doesn't include all the js
(Reporter)

Updated

12 years ago
Attachment #201919 - Attachment description: warning! this will crash your browser! → oops, doesn't include all the js
Attachment #201919 - Attachment is obsolete: true
(Reporter)

Comment 2

12 years ago
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.
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]

Updated

12 years ago
Assignee: mikepinkerton → general
Status: UNCONFIRMED → NEW
Component: General → DOM: HTML
Ever confirmed: true
Flags: blocking1.8rc2?
Keywords: regression, testcase
OS: MacOS X → All
Product: Camino → Core
QA Contact: ian
Version: unspecified → Trunk

Updated

12 years ago
Hardware: Macintosh → All
Created attachment 201928 [details]
Minimal testcase that crashes
Attachment #201921 - Attachment is obsolete: true
Attachment #201925 - Attachment is obsolete: true

Updated

12 years ago
Keywords: crash
Summary: Loading this url crashes Camino and Firefox → Loading this url crashes [@ nsHTMLDocument::MatchLinks] Camino and Firefox
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.
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-10-18+13%3A00%3A00&maxdate=2005-10-19+07%3A00%3A00&cvsroot=%2Fcvsroot
Somehow a regression from fixing bug 312847?

Comment 6

12 years ago
I am crashing with both recent branch and trunk builds but not crashing with the url you posted. 
Flags: blocking1.9a1?

Comment 7

12 years ago
Bz - is this something you can take a look at?
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

12 years ago
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.
Flags: blocking1.8rc2? → blocking1.8rc2-
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.
Created attachment 202018 [details] [diff] [review]
Branch-safe patch
Attachment #202018 - Flags: superreview?(jst)
Attachment #202018 - Flags: review?(jst)

Comment 12

12 years ago
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
Flags: blocking1.8.1?
Blocks: 315999
Blocks: 312695

Comment 13

12 years ago
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

12 years ago
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 on attachment 202018 [details] [diff] [review]
Branch-safe patch

r+sr=jst
Attachment #202018 - Flags: superreview?(jst)
Attachment #202018 - Flags: superreview+
Attachment #202018 - Flags: review?(jst)
Attachment #202018 - Flags: review+
Fixed on trunk
Assignee: general → bzbarsky
Priority: -- → P1
Target Milestone: --- → mozilla1.8final
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.
Attachment #202018 - Flags: approval1.8rc2?

Comment 18

12 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 19

12 years ago
Comment on attachment 202018 [details] [diff] [review]
Branch-safe patch

my approval didn't stick.
Attachment #202018 - Flags: approval1.8rc2? → approval1.8rc2+

Comment 20

12 years ago
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

12 years ago
I landed this on the 1.8 branch for bz. We'll kick off the respins in a few minutes. 



Keywords: fixed1.8
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...
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8) Gecko/20051111 Firefox/1.5 ID:2005111116

verified, no crash
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.
Status: RESOLVED → VERIFIED

Comment 25

12 years ago
fixed for all testcases.
Flags: blocking1.9a1?
Flags: blocking1.8.1?

Updated

9 years ago
Component: DOM: HTML → DOM: Core & HTML
QA Contact: ian → general
Crash Signature: [@ nsHTMLDocument::MatchLinks]
You need to log in before you can comment on or make changes to this bug.