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)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.8final

People

(Reporter: bugzilla, Assigned: bzbarsky)

References

(
URL
)

Details

(4 keywords)

Crash Data

Attachments

(2 files, 3 obsolete files)

Minimal testcase that crashes
350 bytes, text/html
Details
Branch-safe patch
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?  =)
Attached file oops, doesn't include all the js (obsolete) — 

    
  

        Attachment #201919 -
        Attachment description: warning!  this will crash your browser! → oops, doesn't include all the js

        Attachment #201919 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached file
          
        warning, this may crash your browser! (obsolete)
        — 
      

    


  
Oops, left out one of the included js files.  Now they are all in the <head> and the attached html will crash my browser.

    
    

    
  

      
      
      
      

        Attached file
          
        Not minimal testcase (obsolete)
        — 
      

    


  
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]
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
Hardware: Macintosh → All

    
    

    
  


  

        Attachment #201921 -
        Attachment is obsolete: true

        Attachment #201925 -
        Attachment is obsolete: true
Keywords: crash
Summary: Loading this url crashes Camino and Firefox → Loading this url crashes [@ nsHTMLDocument::MatchLinks] Camino and Firefox

    
    

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

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  


  

        Attachment #202018 -
        Flags: superreview?(jst)

        Attachment #202018 -
        Flags: review?(jst)

    
    

    
  
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

    
    

    
  
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.

    
    

    
  
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 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
Closed: 19 years ago
Resolution: --- → FIXED

    
    

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

my approval didn't stick.

        Attachment #202018 -
        Flags: approval1.8rc2? → approval1.8rc2+

    
    

    
  
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.

    
    

    
  
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

    
    

    
  
fixed for all testcases.
Flags: blocking1.9a1?

    
  
Flags: blocking1.8.1?

    
  
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.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: