Found by BlackBerry Security Automated Analysis Team's fuzzing framework ALF. ==29634==ERROR: AddressSanitizer: SEGV on unknown address 0x00000000009b (pc 0x7f97d08bab32 sp 0x7fff7b812210 bp 0x625000c66518 T0) AddressSanitizer can not provide additional info. #0 0x7f97d08bab31 (libxul.so!nsStyleSet::ReparentStyleContext(nsStyleContext*, nsStyleContext*, mozilla::dom::Element*)+0x611) Line 135 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/style/nsStyleContext.h" #1 0x7f97d02a2880 (libxul.so!mozilla::ElementRestyler::RestyleSelf(nsIFrame*, nsRestyleHint)+0xb40) Line 2312 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/RestyleManager.cpp" #2 0x7f97d02a1bed (libxul.so!mozilla::ElementRestyler::Restyle(nsRestyleHint)+0x2cd) Line 2205 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/RestyleManager.cpp" #3 0x7f97d02a23cb (libxul.so!mozilla::ElementRestyler::RestyleSelf(nsIFrame*, nsRestyleHint)+0x68b) Line 2263 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/RestyleManager.cpp" #4 0x7f97d02a1bed (libxul.so!mozilla::ElementRestyler::Restyle(nsRestyleHint)+0x2cd) Line 2205 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/RestyleManager.cpp" #5 0x7f97d02a6916 (libxul.so!mozilla::ElementRestyler::RestyleContentChildren(nsIFrame*, nsRestyleHint)+0x1716) Line 2736 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/RestyleManager.cpp" #6 0x7f97d02a3b50 (libxul.so!mozilla::ElementRestyler::RestyleChildren(nsRestyleHint)+0xc0) Line 2469 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/RestyleManager.cpp" #7 0x7f97d02a1c69 (libxul.so!mozilla::ElementRestyler::Restyle(nsRestyleHint)+0x349) Line 2209 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/RestyleManager.cpp" #8 0x7f97d02a6916 (libxul.so!mozilla::ElementRestyler::RestyleContentChildren(nsIFrame*, nsRestyleHint)+0x1716) Line 2736 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/RestyleManager.cpp" #9 0x7f97d02a3b50 (libxul.so!mozilla::ElementRestyler::RestyleChildren(nsRestyleHint)+0xc0) Line 2469 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/RestyleManager.cpp" #10 0x7f97d02a1c69 (libxul.so!mozilla::ElementRestyler::Restyle(nsRestyleHint)+0x349) Line 2209 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/RestyleManager.cpp" #11 0x7f97d02a6916 (libxul.so!mozilla::ElementRestyler::RestyleContentChildren(nsIFrame*, nsRestyleHint)+0x1716) Line 2736 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/RestyleManager.cpp" #12 0x7f97d02a3b50 (libxul.so!mozilla::ElementRestyler::RestyleChildren(nsRestyleHint)+0xc0) Line 2469 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/RestyleManager.cpp" #13 0x7f97d02a1c69 (libxul.so!mozilla::ElementRestyler::Restyle(nsRestyleHint)+0x349) Line 2209 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/RestyleManager.cpp" #14 0x7f97d02a6916 (libxul.so!mozilla::ElementRestyler::RestyleContentChildren(nsIFrame*, nsRestyleHint)+0x1716) Line 2736 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/RestyleManager.cpp" #15 0x7f97d02a3b50 (libxul.so!mozilla::ElementRestyler::RestyleChildren(nsRestyleHint)+0xc0) Line 2469 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/RestyleManager.cpp" #16 0x7f97d02a1c69 (libxul.so!mozilla::ElementRestyler::Restyle(nsRestyleHint)+0x349) Line 2209 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/RestyleManager.cpp" #17 0x7f97d02a6916 (libxul.so!mozilla::ElementRestyler::RestyleContentChildren(nsIFrame*, nsRestyleHint)+0x1716) Line 2736 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/RestyleManager.cpp" #18 0x7f97d02a3b50 (libxul.so!mozilla::ElementRestyler::RestyleChildren(nsRestyleHint)+0xc0) Line 2469 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/RestyleManager.cpp" #19 0x7f97d02a1c69 (libxul.so!mozilla::ElementRestyler::Restyle(nsRestyleHint)+0x349) Line 2209 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/RestyleManager.cpp" #20 0x7f97d029b8cb (libxul.so!mozilla::RestyleManager::ComputeStyleChangeFor(nsIFrame*, nsStyleChangeList*, nsChangeHint, mozilla::RestyleTracker&, bool)+0x78b) Line 2824 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/RestyleManager.cpp" #21 0x7f97d029aaa9 (libxul.so!mozilla::RestyleManager::RestyleElement(mozilla::dom::Element*, nsIFrame*, nsChangeHint, mozilla::RestyleTracker&, bool)+0x1e9) Line 828 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/RestyleManager.cpp" #22 0x7f97d02ad601 (libxul.so!mozilla::RestyleTracker::DoProcessRestyles()+0xb31) Line 121 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/RestyleTracker.cpp" #23 0x7f97d029ef86 (libxul.so!mozilla::RestyleManager::ProcessPendingRestyles()+0x126) Line 230 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/RestyleTracker.h" #24 0x7f97d0435c2c (libxul.so!PresShell::FlushPendingNotifications(mozilla::ChangesToFlush)+0x70c) Line 3830 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/nsPresShell.cpp" #25 0x7f97d046aa5d (libxul.so!nsRefreshDriver::Tick(long, mozilla::TimeStamp)+0x232d) Line 1139 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/nsRefreshDriver.cpp" #26 0x7f97d0470f00 (libxul.so!mozilla::RefreshDriverTimer::Tick()+0x1f0) Line 168 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/nsRefreshDriver.cpp" #27 0x7f97d4008ee1 (libxul.so!nsTimerImpl::Fire()+0x6d1) Line 546 of "/builds/slave/m-in-l64-asan-0000000000000000/build/xpcom/threads/nsTimerImpl.cpp" #28 0x7f97d4009586 (libxul.so!nsTimerEvent::Run()+0x66) Line 630 of "/builds/slave/m-in-l64-asan-0000000000000000/build/xpcom/threads/nsTimerImpl.cpp" #29 0x7f97d3ffede9 (libxul.so!nsThread::ProcessNextEvent(bool, bool*)+0xaa9) Line 622 of "/builds/slave/m-in-l64-asan-0000000000000000/build/xpcom/threads/nsThread.cpp" #30 0x7f97d3f26f01 (libxul.so!NS_ProcessNextEvent(nsIThread*, bool)+0xb1) Line 238 of "/builds/slave/m-in-l64-asan-0000000000000000/build/xpcom/glue/nsThreadUtils.cpp" #31 0x7f97d2bb2481 (libxul.so!mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*)+0x311) Line 85 of "/builds/slave/m-in-l64-asan-0000000000000000/build/ipc/glue/MessagePump.cpp" #32 0x7f97d411cd43 (libxul.so!MessageLoop::Run()+0x1c3) Line 220 of "/builds/slave/m-in-l64-asan-0000000000000000/build/ipc/chromium/src/base/message_loop.cc" #33 0x7f97d2985d5c (libxul.so!nsBaseAppShell::Run()+0x5c) Line 161 of "/builds/slave/m-in-l64-asan-0000000000000000/build/widget/xpwidgets/nsBaseAppShell.cpp" #34 0x7f97d2371fee (libxul.so!nsAppStartup::Run()+0xbe) Line 268 of "/builds/slave/m-in-l64-asan-0000000000000000/build/toolkit/components/startup/nsAppStartup.cpp" #35 0x7f97cf7fd3f5 (libxul.so!XREMain::XRE_mainRun()+0x1e05) Line 3886 of "/builds/slave/m-in-l64-asan-0000000000000000/build/toolkit/xre/nsAppRunner.cpp" #36 0x7f97cf7fe32a (libxul.so!XREMain::XRE_main(int, char**, nsXREAppData const*)+0x4fa) Line 3954 of "/builds/slave/m-in-l64-asan-0000000000000000/build/toolkit/xre/nsAppRunner.cpp" #37 0x7f97cf7ff25b (libxul.so!XRE_main+0x3ab) Line 4156 of "/builds/slave/m-in-l64-asan-0000000000000000/build/toolkit/xre/nsAppRunner.cpp" #38 0x459b8d (firefox!main+0x94d) Line 275 of "/builds/slave/m-in-l64-asan-0000000000000000/build/browser/app/nsBrowserApp.cpp" #39 0x7f97defa976c (libc.so.6!__libc_start_main+0xec) Line 226 of "libc-start.c" #40 0x45910c (firefox!_start+0x28) ==29634==ABORTING

Regression window(m-c) Good: http://hg.mozilla.org/mozilla-central/rev/a79132ac2f05 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120816175051 Bad: http://hg.mozilla.org/mozilla-central/rev/e1cd9fb39dd7 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120817052252 Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a79132ac2f05&tochange=e1cd9fb39dd7 Regression window(m-i) Good: http://hg.mozilla.org/integration/mozilla-inbound/rev/303b75594832 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120813210542 Bad: http://hg.mozilla.org/integration/mozilla-inbound/rev/4ddd9c731adc Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120813210943 Pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=303b75594832&tochange=4ddd9c731adc Regressed by: bcac58cbf328 Chris Pearce — Bug 775965 - Ensure presentation persists across nsSubDocumentFrame reframes. r=roc

Take with a grain of salt, but I've had a bunch of crashes with this signature happen to me (e.g. bp-e415adb9-0d3e-4440-93f9-198f92140226 ) when opening a new URL through an external program. But it only happens sporadically.

Interesting, it seems there a bunch of bugs for crashes with this signature. I've only seen it with OMTC enabled (bug 950052). It seems aNewParentContext can sometimes be null and the code doesn't handle this.

How can aNewParentContext be null?

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #5) > How can aNewParentContext be null? Or, more accurately, how can aNewParentContext be null when the old parent context is not? Since aNewParentContext being null when the old parent is also null is handled just fine. I also don't think the caller in this stack should need it. That said, it looks like that may well be what's happening here.

The underlying problem is that we appear to have an applet whose xbl-created (pluginProblems.xml#pluginProblem, I think) html:div child (which is display:table, for kicks) has been unbound from the tree, yet the frame is still in the frame tree.

We had an UnbindFromTree call on the element in question (the one that stil has a frame) here (most recently, anyway): nsGenericHTMLElement::UnbindFromTree(bool, bool) (/home/dbaron/builds/ssd/mozilla-central/mozilla/content/html/content/src/nsGenericHTMLElement.cpp:646) nsXBLBinding::UninstallAnonymousContent(nsIDocument*, nsIContent*) (/home/dbaron/builds/ssd/mozilla-central/mozilla/dom/xbl/nsXBLBinding.cpp:306) nsRefPtr<mozilla::dom::XBLChildrenElement>::get() const (/home/dbaron/builds/ssd/mozilla-central/obj/firefox-debugopt/dom/xbl/../../dist/include/nsAutoPtr.h:1029) nsBindingManager::RemovedFromDocumentInternal(nsIContent*, nsIDocument*) (/home/dbaron/builds/ssd/mozilla-central/mozilla/dom/xbl/nsBindingManager.cpp:337) RemoveFromBindingManagerRunnable::Run() (/home/dbaron/builds/ssd/mozilla-central/mozilla/content/base/src/Element.cpp:1264) nsCOMPtr<nsIRunnable>::assign_with_AddRef(nsISupports*) (/home/dbaron/builds/ssd/mozilla-central/obj/firefox-debugopt/content/base/src/../../../dist/include/nsCOMPtr.h:1194) nsDocument::EndUpdate(unsigned int) (/home/dbaron/builds/ssd/mozilla-central/mozilla/content/base/src/nsDocument.cpp:4699) nsHTMLDocument::EndUpdate(unsigned int) (/home/dbaron/builds/ssd/mozilla-central/mozilla/content/html/document/src/nsHTMLDocument.cpp:2462) nsHtml5TreeOpExecutor::RunFlushLoop() (/home/dbaron/builds/ssd/mozilla-central/mozilla/parser/html/nsHtml5TreeOpExecutor.cpp:556) nsHtml5ExecutorFlusher::Run() (/home/dbaron/builds/ssd/mozilla-central/mozilla/parser/html/nsHtml5StreamParser.cpp:131) nsThread::ProcessNextEvent(bool, bool*) (/home/dbaron/builds/ssd/mozilla-central/mozilla/xpcom/threads/nsThread.cpp:643) NS_ProcessNextEvent(nsIThread*, bool) (/home/dbaron/builds/ssd/mozilla-central/mozilla/xpcom/glue/nsThreadUtils.cpp:263) mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (/home/dbaron/builds/ssd/mozilla-central/mozilla/ipc/glue/MessagePump.cpp:96) MessageLoop::RunInternal() (/home/dbaron/builds/ssd/mozilla-central/mozilla/ipc/chromium/src/base/message_loop.cc:227) ~AutoRunState (/home/dbaron/builds/ssd/mozilla-central/mozilla/ipc/chromium/src/base/message_loop.cc:513) nsBaseAppShell::Run() (/home/dbaron/builds/ssd/mozilla-central/mozilla/widget/xpwidgets/nsBaseAppShell.cpp:166) nsAppStartup::Run() (/home/dbaron/builds/ssd/mozilla-central/mozilla/toolkit/components/startup/nsAppStartup.cpp:277) XREMain::XRE_mainRun() (/home/dbaron/builds/ssd/mozilla-central/mozilla/toolkit/xre/nsAppRunner.cpp:3954) XREMain::XRE_main(int, char**, nsXREAppData const*) (/home/dbaron/builds/ssd/mozilla-central/mozilla/toolkit/xre/nsAppRunner.cpp:4020) XRE_main (/home/dbaron/builds/ssd/mozilla-central/mozilla/toolkit/xre/nsAppRunner.cpp:4230) do_main (/home/dbaron/builds/ssd/mozilla-central/mozilla/browser/app/nsBrowserApp.cpp:282) main (/home/dbaron/builds/ssd/mozilla-central/mozilla/browser/app/nsBrowserApp.cpp:643)

The basic problem here is that we have a RemoveFromBindingManagerRunnable (created in the UnbindFromTree call for an applet element) that doesn't get a chance to run until after the same applet has had BindToTree called on it again. Thus we end up unbinding the anonymous content when it has frames associated with it, and then we end up doing a restyle starting from an ancestor of those frames. I stuck in some bogus assertions to get stacks; here's the sequence of stacks, showing all the calls to BindToTree and UnbindFromTree, and the Run method of the runnable, for that applet element. Also everything else in between (not all that much), until the restyle happens (which asserts because the element's GetCurrentDoc() is null). This code was added in bug 736695 part 3.

Hmm. So it looks like we have a eTreeOpDetach which removes the node (with a scriptblocker on the stack?), and then we append it. And then we process a eTreeOpAppendChildrenToNewParent which also removes and appends. The script blocker doesn't come off the stack until we unwind to RunFlushLoop. There is no way to get this sort of access pattern normally, I believe; can we just make the parser behave more like normal DOM mutations here?

(In reply to Boris Zbarsky [:bz] (reviews will be slow; ask someone else) from comment #10) > Hmm. So it looks like we have a eTreeOpDetach which removes the node (with > a scriptblocker on the stack?), and then we append it. And then we process > a eTreeOpAppendChildrenToNewParent which also removes and appends. The > script blocker doesn't come off the stack until we unwind to RunFlushLoop. This is very much by-design. However, it's been some years between the design and now, so my memory is a bit fuzzy on what exactly motivated the design. For sure, document.write() was a large part of it. If the parser was able to cause synchronous script execution in cases other than </script>, document.write() would misbehave per spec. Though the problem here seems to be XBL (will we ever get rid of XBL?), I suppose we could say that XBL lives outside Web standards and that XBL-caused script execution that's synchronous with parsing doesn't obey the standard rules for synchronous parser-invoked scripting and, therefore, document.write() blows away the document. However, since my memory is fuzzy and the parser has very deliberately been written with the assumption that scripts aren't allowed to run at arbitrary points, I don't have a good idea of what else might break if the relationship of the parser flush loop and script execution changes. > There is no way to get this sort of access pattern normally, I believe; can > we just make the parser behave more like normal DOM mutations here? It seems sad to be less able to reason about the world in the parser in order to cater for XBL... I'd hope we'd head to the direction of making the points at which scripts may run more constrained than relaxing the constraints we already have.

The problem is that for XBL we need to tear down some state on removal from the DOM. We used to do it immediately when the removal happened, but the XBL teardown includes running arbitrary JS (effectively; extension-provided JS). It's not clear to me whether custom elements will be any better here, by the way. They have "left view" callbacks that fire when you remove them from the DOM. Now clearly running script of either sort is not OK while a scriptblocker is on the stack, so we have to do it when the last scriptblocker comes off the stack. For the custom element case, it's not clear to me that anyone has really stopped to think about how it should behave with the adoption agency... In any case, if we can't change the parser, we need to try to fix XBL to deal somehow. In this case "deal" probably means tearing down the old binding, not any new one we might have picked up. Blake, you've poked at the anon content end of XBL most recently; are you willing to take a look?

(In reply to Boris Zbarsky [:bz] (reviews will be slow; ask someone else) from comment #12) > In any case, if we can't change the parser We might be able to. We can address the document.write() case by making document.write() from these scripts blow away the document the way document.write() from setTimeout() would. Having thought about this more, I'm pretty sure that the current design arose before the document.write() behavior from non-</script> sources was well understood. However, I can't recall if there are other issues besides document.write() that motivated the current behavior or that unknowingly depend on it, so a change worries me, even though I can't think of any show stopper in particular. So if XBL can deal, I'd prefer not taking risks because of XBL.

(In reply to Boris Zbarsky [:bz] (reviews will be slow; ask someone else) from comment #10) > There is no way to get this sort of access pattern normally, I believe; can > we just make the parser behave more like normal DOM mutations here? I agree with Henri that we should avoid more script execution points, especially not in between the tree ops that you mentioned because they are generated by certain steps of the adoption agency algorithm and it's probably too big of an opportunity to violate invariants if we let script change things in between those tree ops. (In reply to Boris Zbarsky [:bz] (reviews will be slow; ask someone else) from comment #12) > Now clearly running script of either sort is not OK while a scriptblocker is > on the stack, so we have to do it when the last scriptblocker comes off the > stack. For the custom element case, it's not clear to me that anyone has > really stopped to think about how it should behave with the adoption > agency... This isn't an issue for custom elements (as it's currently specified) because callbacks other than the created callback (such as attached and detached callbacks) can not be enqueued until after the created callback is invoked. The first opportunity for invocation is at the microtask checkpoint. (In reply to Boris Zbarsky [:bz] (reviews will be slow; ask someone else) from comment #12) > In any case, if we can't change the parser, we need to try to fix XBL to > deal somehow. In this case "deal" probably means tearing down the old > binding, not any new one we might have picked up. I looked into this and it turns out that if RemoveFromBindingManagerRunnable doesn't get run, the binding on the element will stay the same when it is reinserted back into the document because nsXBLService::LoadBinding has an early return check for when we try to load the same binding (which is the case in this bug), so there is nothing to tear down. nsXBLService::LoadBinding also has some code to tear down the old binding before setting the new binding in the case we try to load a different binding. Although I'm not sure if it's even possible to load a different binding before RemoveFromBindingManagerRunnable has run. In either case, it seems sufficient to ensure that we don't tear down bindings when the element is in the document.

(In reply to Henri Sivonen (:hsivonen) from comment #13) > However, I can't recall if there are other issues besides document.write() > that motivated the current behavior or that unknowingly depend on it, so a > change worries me, It turns out that while our innerHTML implementation didn't motivate the current design, the innerHTML optimization of parsing directly into the context node was introduced after the current design design and depends on the current design. That is, if scripts could run as a side effect of parser-performed insertions, Web pages could detect that we don't actually implement innerHTML as a parse to a fragment followed by the insertion of the fragment.

Comment on attachment 8387330 [details] [diff] [review] Don't tear down XBL binding when element is in document. Hmm, ok. As long as this also works right if the element has been adopted and inserted into a different document...