Closed
Bug 315189
Opened 19 years ago
Closed 19 years ago
Loading this url crashes [@ nsHTMLDocument::MatchLinks] Camino and Firefox
Categories
(Core :: DOM: Core & HTML, defect, P1)
Core
DOM: Core & HTML
Tracking
()
VERIFIED
FIXED
mozilla1.8final
People
(Reporter: bugzilla, Assigned: bzbarsky)
References
()
Details
(4 keywords)
Crash Data
Attachments
(2 files, 3 obsolete files)
350 bytes,
text/html
|
Details | |
4.08 KB,
patch
|
jst
:
review+
jst
:
superreview+
mscott
:
approval1.8rc2+
|
Details | Diff | Splinter Review |
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? =)
Attachment #201919 -
Attachment description: warning! this will crash your browser! → oops, doesn't include all the js
Attachment #201919 -
Attachment is obsolete: true
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•19 years ago
|
||
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•19 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•19 years ago
|
Hardware: Macintosh → All
Comment 4•19 years ago
|
||
Attachment #201921 -
Attachment is obsolete: true
Attachment #201925 -
Attachment is obsolete: true
Updated•19 years ago
|
Keywords: crash
Summary: Loading this url crashes Camino and Firefox → Loading this url crashes [@ nsHTMLDocument::MatchLinks] Camino and Firefox
Comment 5•19 years ago
|
||
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•19 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•19 years ago
|
||
Bz - is this something you can take a look at?
Assignee | ||
Comment 8•19 years ago
|
||
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•19 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-
Assignee | ||
Comment 10•19 years ago
|
||
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.
Assignee | ||
Comment 11•19 years ago
|
||
Attachment #202018 -
Flags: superreview?(jst)
Attachment #202018 -
Flags: review?(jst)
Comment 12•19 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
Assignee | ||
Updated•19 years ago
|
Flags: blocking1.8.1?
Comment 13•19 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•19 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 15•19 years ago
|
||
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+
Assignee | ||
Comment 16•19 years ago
|
||
Fixed on trunk
Assignee: general → bzbarsky
Priority: -- → P1
Target Milestone: --- → mozilla1.8final
Assignee | ||
Comment 17•19 years ago
|
||
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•19 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.
Assignee | ||
Updated•19 years ago
|
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Comment 19•19 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•19 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•19 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
Assignee | ||
Comment 22•19 years ago
|
||
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•19 years ago
|
||
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
Assignee | ||
Updated•19 years ago
|
Flags: blocking1.8.1?
Updated•14 years ago
|
Crash Signature: [@ nsHTMLDocument::MatchLinks]
You need to log in
before you can comment on or make changes to this bug.
Description
•