Last Comment Bug 691215 - crash nsXMLPrettyPrinter::Unhook , I believe document.replaceChild (in bookmarklet) crashes FF 7.x
: crash nsXMLPrettyPrinter::Unhook , I believe document.replaceChild (in bookm...
Status: RESOLVED FIXED
: crash, regression
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86 Windows 7
: P1 critical (vote)
: mozilla10
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
javascript:(function(){function%20j(b...
: 714232 (view as bug list)
Depends on:
Blocks: 657353
  Show dependency treegraph
 
Reported: 2011-10-02 17:50 PDT by J Del Monte
Modified: 2011-12-29 21:45 PST (History)
6 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
FF_fail.txt (5.49 KB, text/plain)
2011-10-02 17:50 PDT, J Del Monte
no flags Details
xml (1.24 KB, text/xml)
2011-10-02 18:31 PDT, Alice0775 White
no flags Details
Fix stupid logic error in the XML prettyprinter that can cause a null-dereference crash. (3.31 KB, patch)
2011-10-02 20:13 PDT, Boris Zbarsky [:bz]
jonas: review+
christian: approval‑mozilla‑aurora-
Details | Diff | Review

Description J Del Monte 2011-10-02 17:50:01 PDT
Created attachment 564094 [details]
FF_fail.txt

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0.1) Gecko/20100101 Firefox/7.0.1
Build ID: 20110928134238

Steps to reproduce:

Tried using a bookmarklet that worked fine in FF 6.x


Actual results:

FF crashes (tried in both W2K and 64 bit Win 7)


Expected results:

Should have worked - returns a link from within the XML document.   I think it fails at: document.replaceChild(f,document.firstChild)
Comment 1 Tim (fmdeveloper) 2011-10-02 17:59:05 PDT
Please provide a crash id or stacktrace -> https://developer.mozilla.org/en/How_to_get_a_stacktrace_for_a_bug_report

If you could reduce the test case to the bare minimum that would also help
Comment 2 J Del Monte 2011-10-02 18:22:51 PDT
(In reply to Tim (fmdeveloper) from comment #1)
> Please provide a crash id or stacktrace ->
> https://developer.mozilla.org/en/How_to_get_a_stacktrace_for_a_bug_report
> 
> If you could reduce the test case to the bare minimum that would also help

Hopefully this is what you are looking for:
http://crash-stats.mozilla.com/report/index/bp-e5f85efa-050b-402e-ba2b-513b82111002

By test case do you mean the XML document or the bookmarklet?   The XML sample is what the portable ding returns so it is a true test case.   I didn't write the bookmarlket from heck so I cannot really trim it, sorry.
Comment 3 Alice0775 White 2011-10-02 18:29:58 PDT
bp-30247a53-b238-421d-b562-680b12111002


Regression Window(cached m-c hourly),
Works:
http://hg.mozilla.org/mozilla-central/rev/5c6d107ede5a
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0a1) Gecko/20110601 Firefox/7.0a1 ID:20110601030746
Crashes:
http://hg.mozilla.org/mozilla-central/rev/840644b2b6a2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0a1) Gecko/20110601 Firefox/7.0a1 ID:20110601011530
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5c6d107ede5a&tochange=840644b2b6a2

Triggered by:
Bug 657353 - Stop sending BeginUpdate/EndUpdate on content state changes
Comment 4 Alice0775 White 2011-10-02 18:31:21 PDT
Created attachment 564100 [details]
xml
Comment 5 Alice0775 White 2011-10-02 18:37:42 PDT
[str]
1.open attachment 564100 [details]
2.the following bookmarklet should be bookmarked,
3.open the bookmarklet.

[actual]
browser crashes

javascript:(function(){function%20j(b,a,c){typeof%20c==="undefined"&&(c=document);typeof%20a==="undefined"&&(a=0);b=c.getElementsByTagName(b);return%20b.length<a+1?"":b[a].firstChild.data}function%20h(b,a,c){return{attrName:b,attrVal:a,text:c}}function%20g(b){var%20a=document.createElementNS("http://www.w3.org/1999/xhtml",b),c;arguments.length>1&&(c=arguments[1],c.attrName.length&&c.attrVal.length&&a.setAttribute(c.attrName,c.attrVal),c.text.length&&a.appendChild(document.createTextNode(c.text)));return%20a}function%20i(b){for(var%20a=%20b.length-1;a>0;a--)b[a-1].appendChild(b[a])}(function(){for(var%20b=void%200,a=void%200,c=void%200,b=b?b:a?a.ownerDocument:document,f=[],a=b.evaluate("//headline",a?a:b,c?c:null,XPathResult.ORDERED_NODE_SNAPSHOT_TYPE,null),b=0;b<a.snapshotLength;b++)f[b]=a.snapshotItem(b);var%20d,e,k,l;if(f.length===0)alert("No%20messages!");else{e=window.location.toString().replace("getMessages","createSession");window.XMLHttpRequest?(xhttp=new%20XMLHttpRequest,xhttp.open("GET",e,!1),xhttp.send(""),e=xhttp.responseXML):(alert("Sorry,%20your%20browswer%20can't%20handle%20this!"),%20e=!1);j("status",0,e)!=="0"?alert("Unable%20to%20create%20session;%20verify%20installId%20and%20cksum"):d=j("disc",0,e);b="&disc="+d;e=g("ul",h("style","margin:2em;",""),"");for(d=0;d<f.length;d++)a=j("activationText",0,f[d]).replace(/(<([^>]+)>)/ig,""),c=j("activationUrl",0,f[d]),c.length>0?(j("type",0,f[d])==="offer"?(c+=b,k=j("activationNewDelta",0,f[d])*1,l=g("span",h("style","margin-left:2em;%20font-size:80%;","Active%20in%20"+(k/60).toFixed(0)+":"+((k%%2060)/100).toFixed(2)*100))):k=0,i([e,g("li"),g("a",h("href",c,%20a))]),k>0&&i([e.lastChild,l])):i([e,g("li",h("","",a))])}f=g("div");i([f,e]);l=g("html:div",h("style","margin:%202.5em%205em;%20color:%20red;%20font-size:%2090%;%20font-style:%20italic;",""));e=g("p",h("style","padding:%205px;",""));d=g("span",h("style","border:%201px%20solid%20black;%20padding:2px;","Caution:%20You%20are%20using%20Beta%20v0.1%20of%20this%20script.%20Be%20sure%20to%20"));d.appendChild(g("a",h("href","http://www.flyertalk.com/forum/16382603-post8.html","check%20periodically")));d.appendChild(document.createTextNode("%20for%20an%20updated%20version."));%20i([l,e,d]);i([f,l]);i([f,g("p",h("style","font-size:80%;%20margin:2em;",document.lastModified))]);document.replaceChild(f,document.firstChild)})()})();
Comment 6 Alice0775 White 2011-10-02 18:41:54 PDT
Sorry the above bookmarklet fails to run due to cut and paste problem or something.

Pls use bookmarklet described in attachment 564094 [details]
Comment 7 Boris Zbarsky [:bz] 2011-10-02 19:52:29 PDT
So mDocument is null in nsXMLPrettyPrinter::Unhook.

What happens is that the replaceChild call triggers a remove and an insert which both call MaybeUnhook.  So we get two runnables to unhook posted, and then the second one of those to run loses.

And the reason we get two runnables posted is because the check is like so:

    if (!aContent || !aContent->GetBindingParent() && !mUnhookPending) {

which is clearly bogus; it tests true any time !aContent, which is the case here because aContent is the aContainer from ContentInserted/ContentRemoved, which is null for replacement of the document element.
Comment 8 Boris Zbarsky [:bz] 2011-10-02 20:13:29 PDT
Created attachment 564109 [details] [diff] [review]
Fix stupid logic error in the XML prettyprinter that can cause a null-dereference crash.
Comment 9 Boris Zbarsky [:bz] 2011-10-02 20:14:43 PDT
Thank you all for the clear steps to reproduce!
Comment 10 Jonas Sicking (:sicking) 2011-10-02 22:18:59 PDT
Comment on attachment 564109 [details] [diff] [review]
Fix stupid logic error in the XML prettyprinter that can cause a null-dereference crash.

Thanks!
Comment 12 Boris Zbarsky [:bz] 2011-10-03 12:18:30 PDT
Comment on attachment 564109 [details] [diff] [review]
Fix stupid logic error in the XML prettyprinter that can cause a null-dereference crash.

It may be worth taking this on aurora.  This is a very safe change that fixes a null-deref crash that web content can actually trigger pretty easily (see the mochitest).

On the other hand, I expect our frequency for this crash is pretty low...
Comment 13 Matt Brubeck (:mbrubeck) 2011-10-03 16:52:14 PDT
https://hg.mozilla.org/mozilla-central/rev/5a26e1aa86e5
Comment 14 christian 2011-10-06 14:45:49 PDT
Comment on attachment 564109 [details] [diff] [review]
Fix stupid logic error in the XML prettyprinter that can cause a null-dereference crash.

Crash level is very, very low. We'll let this come up in the normal process.
Comment 15 Boris Zbarsky [:bz] 2011-12-29 21:45:27 PST
*** Bug 714232 has been marked as a duplicate of this bug. ***

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