1482402 - message-manager-close observer notification can cause us to dispatch DOM events at unsafe times

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

This was revealed in bug 1451108 when switching us to fatal assertions when dispatching DOM events at unsafe times.

Apparently, some observer of the message-manager-close notification is causing an event to be dispatched. We need to audit them and find it.

Comment 1

[Andrew McCreight [:mccr8]]

How bad is this, security-wise?

Comment 2

[Mike Conley (:mconley) (:⚙️)]

I'm not sure. smaug might have a more informed opinion than me.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Possibly very bad. Running scripts when scripts aren't supposed to run. And it is unknown what the scripts might do. sec-high?

Comment 4

[Mike Conley (:mconley) (:⚙️)]

I got this in rr, and got a JS stack:

0 setIframeVisible(iframe = [object XULFrameElement], visible = false) ["resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/toolbox.js":1836]
    this = [object Object]
1 selectSingleNode/<(node = [object XULElement], 9, [object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement]) ["resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/toolbox.js":1811]
2 forEach(callbackfn = node => {
      if (node.id === id) {
        node.setAttribute("selected", "true");
        node.setAttribute("aria-selected", "true");
      } else {
        node.removeAttribute("selected");
        node.removeAttribute("aria-selected");
      }
      // The webconsole panel is in a special location due to split console
      if (!node.id) {
        node = this.webconsolePanel;
      }

      const iframe = node.querySelector(".toolbox-panel-iframe");
      if (iframe) {
        let visible = node.id == id;
        // Prevents hiding the split-console if it is currently enabled
        if (node == this.webconsolePanel && this.splitConsole) {
          visible = true;
        }
        this.setIframeVisible(iframe, visible);
      }
    }) ["self-hosted":268]
    this = [object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement]
3 selectSingleNode(collection = [object NodeList], id = "toolbox-panel-inspector") ["resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/toolbox.js":1791]
    this = [object Object]
4 selectTool(id = "inspector", reason = "tool_unloaded") ["resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/toolbox.js":1886]
    this = [object Object]
5 selectTool("inspector", "tool_unloaded") ["self-hosted":987]

Comment 5

[Mike Conley (:mconley) (:⚙️)]

The message-manager-close notification is firing because of this JS stack:

0 destroyBrowserElement() ["chrome://browser/content/parent/ext-devtools-panels.js":288]
    this = [object Object]
1 buildPanel/<() ["chrome://browser/content/parent/ext-devtools-panels.js":121]
2 unloadTool(toolId = "webext-devtools-panel-_cb6cd29c-2a86-4a9a-922c-e1f522ec8baa_-32-0") ["resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/toolbox.js":2604]
    this = [object Object]
3 removeAdditionalTool(toolId = "webext-devtools-panel-_cb6cd29c-2a86-4a9a-922c-e1f522ec8baa_-32-0") ["resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/toolbox.js":1625]
    this = [object Object]
4 close() ["chrome://browser/content/parent/ext-devtools-panels.js":196]
    this = [object Object]
5 unload() ["resource://gre/modules/ExtensionCommon.jsm":733]
    this = [object Object]
6 unload() ["resource://gre/modules/ExtensionParent.jsm":614]
    this = [object Object]
7 unload() ["resource://gre/modules/ExtensionParent.jsm":696]
    this = [object Object]
8 closeProxyContext(childId = "{cb6cd29c-2a86-4a9a-922c-e1f522ec8baa}.30") ["resource://gre/modules/ExtensionParent.jsm":878]
    this = [object Object]
9 observe() ["resource://gre/modules/ExtensionParent.jsm":777]
10 InterpretGeneratorResume(gen = [object Generator], val = undefined, kind = "next") ["self-hosted":1271]
11 next(val = undefined) ["self-hosted":1226]
    this = [object Generator]
12 shutdown() ["resource://gre/modules/ExtensionParent.jsm":1186]
    this = [object Object]
13 close() ["chrome://browser/content/parent/ext-devtools.js":201]
    this = [object Object]
14 shutdownForTarget(target = TabTarget:[object XULElement]) ["chrome://browser/content/parent/ext-devtools.js":257]
    this = [object Object]
15 onToolboxDestroy(target = TabTarget:[object XULElement]) ["chrome://browser/content/parent/ext-devtools.js":402]
    this = [object Object]
16 onToolboxDestroy(TabTarget:[object XULElement]) ["self-hosted":985]
    this = [object Object]

Comment 6

[Mike Conley (:mconley) (:⚙️)]

At the time of the crash, there are 2 script blockers:

The first comes from this stack:

#1  0x00007f30739d81a5 in nsIDocument::BeginUpdate (this=0x7f305b429000) at /home/mconley/Projects/mozilla-central/dom/base/nsDocument.cpp:4898
#2  0x00007f30737a6ca3 in mozAutoDocUpdate::mozAutoDocUpdate (this=0x7ffc7728adc0, aDocument=0x7f305b429000, aNotify=true) at /home/mconley/Projects/mozilla-central/dom/base/mozAutoDocUpdate.h:28
#3  0x00007f30738c0aeb in mozilla::dom::Element::UnsetAttr (this=0x7f304a92cca0, aNameSpaceID=0, aName=0x7f307b656a70 <mozilla::detail::gGkAtoms+53872>, aNotify=true) at /home/mconley/Projects/mozilla-central/dom/base/Element.cpp:2965
#4  0x00007f30738bb875 in mozilla::dom::Element::RemoveAttribute (this=0x7f304a92cca0, aName=..., aError=...) at /home/mconley/Projects/mozilla-central/dom/base/Element.cpp:1412
#5  0x00007f30746f91c0 in mozilla::dom::Element_Binding::removeAttribute (cx=0x7f306c722000, obj=..., self=0x7f304a92cca0, args=...) at /home/mconley/Projects/mozilla-central/debug/dom/bindings/ElementBinding.cpp:1375
#6  0x00007f30749f1e35 in mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions> (cx=0x7f306c722000, argc=1, vp=0x7ffc7728b4e0) at /home/mconley/Projects/mozilla-central/dom/bindings/BindingUtils.cpp:3296
#7  0x00007f3078772386 in CallJSNative (cx=0x7f306c722000, native=0x7f30749f1b7c <mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*)>, args=...) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:449
#8  0x00007f3078743f26 in js::InternalCallOrConstruct (cx=0x7f306c722000, args=..., construct=js::NO_CONSTRUCT) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:537
#9  0x00007f3078744332 in InternalCall (cx=0x7f306c722000, args=...) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:588
#10 0x00007f30787443ec in js::Call (cx=0x7f306c722000, fval=..., thisv=..., args=..., rval=...) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:607
#11 0x00007f3078db99cb in js::ForwardingProxyHandler::call (this=0x7f307fd19cb0 <js::CrossCompartmentWrapper::singleton>, cx=0x7f306c722000, proxy=..., args=...) at /home/mconley/Projects/mozilla-central/js/src/proxy/Wrapper.cpp:176
#12 0x00007f3078d9f4e0 in js::CrossCompartmentWrapper::call (this=0x7f307fd19cb0 <js::CrossCompartmentWrapper::singleton>, cx=0x7f306c722000, wrapper=..., args=...) at /home/mconley/Projects/mozilla-central/js/src/proxy/CrossCompartmentWrapper.cpp:339
#13 0x00007f3078dafacb in js::Proxy::call (cx=0x7f306c722000, proxy=..., args=...) at /home/mconley/Projects/mozilla-central/js/src/proxy/Proxy.cpp:513
#14 0x00007f3078743cc1 in js::InternalCallOrConstruct (cx=0x7f306c722000, args=..., construct=js::NO_CONSTRUCT) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:513
#15 0x00007f3078744332 in InternalCall (cx=0x7f306c722000, args=...) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:588
#16 0x00007f3078744370 in js::CallFromStack (cx=0x7f306c722000, args=...) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:594
#17 0x00007f30787527c3 in Interpret (cx=0x7f306c722000, state=...) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:3243
#18 0x00007f30787439d4 in js::RunScript (cx=0x7f306c722000, state=...) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:429
#19 0x00007f3078744098 in js::InternalCallOrConstruct (cx=0x7f306c722000, args=..., construct=js::NO_CONSTRUCT) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:561
#20 0x00007f3078744332 in InternalCall (cx=0x7f306c722000, args=...) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:588
#21 0x00007f3078744370 in js::CallFromStack (cx=0x7f306c722000, args=...) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:594
#22 0x00007f30788b3ed6 in js::jit::DoCallFallback (cx=0x7f306c722000, frame=0x7ffc7728c508, stub_=0x7f305905c538, argc=3, vp=0x7ffc7728c468, res=...) at /home/mconley/Projects/mozilla-central/js/src/jit/BaselineIC.cpp:2582

JS stack:

0 selectSingleNode/<(node = [object XULElement], 9, [object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement]) ["resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/toolbox.js":1797]
1 forEach(callbackfn = node => {
      if (node.id === id) {
        node.setAttribute("selected", "true");
        node.setAttribute("aria-selected", "true");
      } else {
        node.removeAttribute("selected");
        node.removeAttribute("aria-selected");
      }
      // The webconsole panel is in a special location due to split console
      if (!node.id) {
        node = this.webconsolePanel;
      }

      const iframe = node.querySelector(".toolbox-panel-iframe");
      if (iframe) {
        let visible = node.id == id;
        // Prevents hiding the split-console if it is currently enabled
        if (node == this.webconsolePanel && this.splitConsole) {
          visible = true;
        }
        this.setIframeVisible(iframe, visible);
      }
    }) ["self-hosted":268]
    this = [object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement]
2 selectSingleNode(collection = [object NodeList], id = "toolbox-panel-inspector") ["resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/toolbox.js":1791]
    this = [object Object]
3 selectTool(id = "inspector", reason = "tool_unloaded") ["resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/toolbox.js":1886]
    this = [object Object]
4 selectTool("inspector", "tool_unloaded") ["self-hosted":987]
    this = [object Object]
5 unloadTool(toolId = "webext-devtools-panel-_e33655ae-71b3-404d-bb42-3b2be1c9cce2_-32-0") ["resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/toolbox.js":2624]


The second from this stack:

#0  nsContentUtils::AddScriptBlocker () at /home/mconley/Projects/mozilla-central/dom/base/nsContentUtils.cpp:5546
#1  0x00007f3076209607 in mozilla::PresShell::WillCauseReflow (this=0x7f305a549000) at /home/mconley/Projects/mozilla-central/debug/dist/include/mozilla/PresShell.h:415
#2  0x00007f307620a8a4 in nsAutoCauseReflowNotifier::nsAutoCauseReflowNotifier (this=0x7ffc7728acb0, aShell=0x7f305a549000) at /home/mconley/Projects/mozilla-central/layout/base/PresShell.cpp:453
#3  0x00007f30761da97c in mozilla::PresShell::AttributeChanged (this=0x7f305a549000, aElement=0x7f304a92cca0, aNameSpaceID=0, aAttribute=0x7f307b656a70 <mozilla::detail::gGkAtoms+53872>, aModType=3, aOldValue=0x7ffc7728add0) at /home/mconley/Projects/mozilla-central/layout/base/PresShell.cpp:4461
#4  0x00007f3073a88d3a in nsNodeUtils::AttributeChanged (aElement=0x7f304a92cca0, aNameSpaceID=0, aAttribute=0x7f307b656a70 <mozilla::detail::gGkAtoms+53872>, aModType=3, aOldValue=0x7ffc7728add0) at /home/mconley/Projects/mozilla-central/dom/base/nsNodeUtils.cpp:170
#5  0x00007f30738c12e7 in mozilla::dom::Element::UnsetAttr (this=0x7f304a92cca0, aNameSpaceID=0, aName=0x7f307b656a70 <mozilla::detail::gGkAtoms+53872>, aNotify=true) at /home/mconley/Projects/mozilla-central/dom/base/Element.cpp:3052
#6  0x00007f30738bb875 in mozilla::dom::Element::RemoveAttribute (this=0x7f304a92cca0, aName=..., aError=...) at /home/mconley/Projects/mozilla-central/dom/base/Element.cpp:1412
#7  0x00007f30746f91c0 in mozilla::dom::Element_Binding::removeAttribute (cx=0x7f306c722000, obj=..., self=0x7f304a92cca0, args=...) at /home/mconley/Projects/mozilla-central/debug/dom/bindings/ElementBinding.cpp:1375
#8  0x00007f30749f1e35 in mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions> (cx=0x7f306c722000, argc=1, vp=0x7ffc7728b4e0) at /home/mconley/Projects/mozilla-central/dom/bindings/BindingUtils.cpp:3296
#9  0x00007f3078772386 in CallJSNative (cx=0x7f306c722000, native=0x7f30749f1b7c <mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*)>, args=...) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:449
#10 0x00007f3078743f26 in js::InternalCallOrConstruct (cx=0x7f306c722000, args=..., construct=js::NO_CONSTRUCT) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:537
#11 0x00007f3078744332 in InternalCall (cx=0x7f306c722000, args=...) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:588
#12 0x00007f30787443ec in js::Call (cx=0x7f306c722000, fval=..., thisv=..., args=..., rval=...) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:607
#13 0x00007f3078db99cb in js::ForwardingProxyHandler::call (this=0x7f307fd19cb0 <js::CrossCompartmentWrapper::singleton>, cx=0x7f306c722000, proxy=..., args=...) at /home/mconley/Projects/mozilla-central/js/src/proxy/Wrapper.cpp:176
#14 0x00007f3078d9f4e0 in js::CrossCompartmentWrapper::call (this=0x7f307fd19cb0 <js::CrossCompartmentWrapper::singleton>, cx=0x7f306c722000, wrapper=..., args=...) at /home/mconley/Projects/mozilla-central/js/src/proxy/CrossCompartmentWrapper.cpp:339
#15 0x00007f3078dafacb in js::Proxy::call (cx=0x7f306c722000, proxy=..., args=...) at /home/mconley/Projects/mozilla-central/js/src/proxy/Proxy.cpp:513
#16 0x00007f3078743cc1 in js::InternalCallOrConstruct (cx=0x7f306c722000, args=..., construct=js::NO_CONSTRUCT) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:513
#17 0x00007f3078744332 in InternalCall (cx=0x7f306c722000, args=...) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:588
#18 0x00007f3078744370 in js::CallFromStack (cx=0x7f306c722000, args=...) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:594
#19 0x00007f30787527c3 in Interpret (cx=0x7f306c722000, state=...) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:3243
#20 0x00007f30787439d4 in js::RunScript (cx=0x7f306c722000, state=...) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:429
#21 0x00007f3078744098 in js::InternalCallOrConstruct (cx=0x7f306c722000, args=..., construct=js::NO_CONSTRUCT) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:561
#22 0x00007f3078744332 in InternalCall (cx=0x7f306c722000, args=...) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:588
#23 0x00007f3078744370 in js::CallFromStack (cx=0x7f306c722000, args=...) at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:594
#24 0x00007f30788b3ed6 in js::jit::DoCallFallback (cx=0x7f306c722000, frame=0x7ffc7728c508, stub_=0x7f305905c538, argc=3, vp=0x7ffc7728c468, res=...) at /home/mconley/Projects/mozilla-central/js/src/jit/BaselineIC.cpp:2582

JS stack:

0 selectSingleNode/<(node = [object XULElement], 9, [object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement]) ["resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/toolbox.js":1797]
1 forEach(callbackfn = node => {
      if (node.id === id) {
        node.setAttribute("selected", "true");
        node.setAttribute("aria-selected", "true");
      } else {
        node.removeAttribute("selected");
        node.removeAttribute("aria-selected");
      }
      // The webconsole panel is in a special location due to split console
      if (!node.id) {
        node = this.webconsolePanel;
      }

      const iframe = node.querySelector(".toolbox-panel-iframe");
      if (iframe) {
        let visible = node.id == id;
        // Prevents hiding the split-console if it is currently enabled
        if (node == this.webconsolePanel && this.splitConsole) {
          visible = true;
        }
        this.setIframeVisible(iframe, visible);
      }
    }) ["self-hosted":268]
    this = [object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement],[object XULElement]
2 selectSingleNode(collection = [object NodeList], id = "toolbox-panel-inspector") ["resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/toolbox.js":1791]
    this = [object Object]
3 selectTool(id = "inspector", reason = "tool_unloaded") ["resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/toolbox.js":1886]
    this = [object Object]
4 selectTool("inspector", "tool_unloaded") ["self-hosted":987]
    this = [object Object]
5 unloadTool(toolId = "webext-devtools-panel-_e33655ae-71b3-404d-bb42-3b2be1c9cce2_-32-0") ["resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/toolbox.js":2624]

Comment 7

[Mike Conley (:mconley) (:⚙️)]

What appears to be happening is that we're removing these attributes:

https://searchfox.org/mozilla-central/rev/c45b9755593ce6cc91f558b21e544f5165752de7/devtools/client/framework/toolbox.js#1796-1797

and we're adding the script blocker in this UnsetAttr method here:

https://searchfox.org/mozilla-central/rev/c45b9755593ce6cc91f558b21e544f5165752de7/dom/base/Element.cpp#2965

which constructs a mozAutoDocUpdate, which adds the script blocker.

All of this is, I think, in the same turn of the event loop that's calling unloadTool, which is what sends out that observer notification.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Here's a try push where I use a ScriptRunner to fire the message-manager-close notification: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a3e7d329e78d9321729bf6ed08684bdbde8834f

Comment 9

[:Gijs (he/him)]

Does this only happen from devtools, or could it conceivably happen from either webpage or non-devtools chrome code?

Comment 10

[Mike Conley (:mconley) (:⚙️)]

It looks like our automated tests cover a single case where this happens, but I suppose it's possible that any of the other message-manager-close observers might also sometimes fire events.

message-manager-close is only ever dispatched in the parent process though, so with e10s enabled, I think potential future cuprits here is from privileged content, like our DevTools stuff.

And while we could audit all of the parent process consumers and ensure that none fire DOM events, I wonder if the more robust solution is to fire the notification only when it's safe to run script. That's what my last try push did, and I just need to figure out the test failures in _that_ push.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Okay, I think I've started to sort this out. Gonna do some more prodding with rr today to make sure I have the story straight, but I have a patch that I think fixes things.

Comment 12

[Mike Conley (:mconley) (:⚙️)]

1. A test WebExtension using the DevTools API shuts down, and we get here:

https://searchfox.org/mozilla-central/rev/d4ef4e9747133aa2914aca2a15cf9df1e42a6aa0/toolkit/components/extensions/ExtensionParent.jsm#1195

so we're destroying the remote XUL browser for the background / DevTools page.

Removing the node puts a script blocker on the stack:

#0  0x00007f464cca2de4 in nsContentUtils::AddScriptBlocker() () at /home/mconley/Projects/mozilla-central/dom/base/nsContentUtils.cpp:5554
#1  0x00007f464cf2d928 in nsIDocument::BeginUpdate() (this=0x7f4634f86000) at /home/mconley/Projects/mozilla-central/dom/base/nsDocument.cpp:5024
#2  0x00007f464ccbeaca in mozAutoDocUpdate::mozAutoDocUpdate(nsIDocument*, bool) (this=0x7ffc2fe55a40, aDocument=0x7f4634f86000, aNotify=true) at /home/mconley/Projects/mozilla-central/dom/base/mozAutoDocUpdate.h:28
#3  0x00007f464cf29fea in nsINode::RemoveChildNode(nsIContent*, bool) (this=0x7f4632aa4d30, aKid=0x7f462146a710, aNotify=true) at /home/mconley/Projects/mozilla-central/dom/base/nsINode.cpp:2029
#4  0x00007f464cf7eda8 in nsINode::RemoveChild(nsINode&, mozilla::ErrorResult&) (this=0x7f4632aa4d30, aOldChild=..., aError=...) at /home/mconley/Projects/mozilla-central/dom/base/nsINode.cpp:577
#5  0x00007f464cf81e8d in nsINode::Remove() (this=0x7f462146a710) at /home/mconley/Projects/mozilla-central/dom/base/nsINode.cpp:1872
#6  0x00007f464dde57b3 in mozilla::dom::Element_Binding::remove(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Element*, JSJitMethodCallArgs const&) (cx=0x7f4646522000, obj=..., self=0x7f462146a710, args=...) at /home/mconley/Projects/mozilla-central/debug/dom/bindings/ElementBinding.cpp:4620
#7  0x00007f464e10e3ef in mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) (cx=0x7f4646522000, argc=0, vp=0x7ffc2fe56030) at /home/mconley/Projects/mozilla-central/dom/bindings/BindingUtils.cpp:3296
#8  0x00007f4652126843 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (cx=0x7f4646522000, native=0x7f464e10e120 <mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*)>, args=...)
    at /home/mconley/Projects/mozilla-central/js/src/vm/Interpreter.cpp:449


2. In the same tick, with the remote <browser> going away, we fire the message-manager-close notification. The DevTools code notices that, and starts tearing down the associated remote iframe that's hosting the WebExtension DevTools panel.

I believe this iframe destruction gets kicked off here: https://searchfox.org/mozilla-central/rev/d4ef4e9747133aa2914aca2a15cf9df1e42a6aa0/devtools/client/framework/toolbox.js#2623

2. Later on in the same function, another tool is selected, since the removed one was just unloaded:

https://searchfox.org/mozilla-central/rev/d4ef4e9747133aa2914aca2a15cf9df1e42a6aa0/devtools/client/framework/toolbox.js#2643

3. This attempts to select the right panel in the list of toolbox panels:

https://searchfox.org/mozilla-central/rev/d4ef4e9747133aa2914aca2a15cf9df1e42a6aa0/devtools/client/framework/toolbox.js#1913

4. We end up dispatching this fake visibilitychange event on the iframe's outer window here: https://searchfox.org/mozilla-central/rev/d4ef4e9747133aa2914aca2a15cf9df1e42a6aa0/devtools/client/framework/toolbox.js#1863

Because the event is not the "void" event, because it's still unsafe to run script (since steps 1-4 all appear to run on the same tick of the event loop), and because the outer window can't be QI'd to an nsINode, we explode here:

https://searchfox.org/mozilla-central/rev/d4ef4e9747133aa2914aca2a15cf9df1e42a6aa0/dom/events/EventDispatcher.cpp#907 (since the patch in bug 1451108 makes this a crash instead of a warning).

Comment 13

[Mike Conley (:mconley) (:⚙️)]

So, to sum, the problem here is that on removing the background page remote <browser>, we fire a message-manager-close notification, which DevTools JS hears and dispatching an event on the outer window of a node that can't QI to nsINode.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1482402 - Dispatch DevTools toolbox visibilitychange event on the iframe document instead of the iframe window. r?gl

Comment 15

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 9007866 [details]
Bug 1482402 - Dispatch DevTools toolbox visibilitychange event on the iframe document instead of the iframe window. r?gl

Gabriel [:gl] (ΦωΦ) has approved the revision.

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 9007866 [details]
Bug 1482402 - Dispatch DevTools toolbox visibilitychange event on the iframe document instead of the iframe window. r?gl

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

  Not easily, I don't think. As far as I can tell, the only thing that might run script on this event is our own DevTools code.

  So I'm not even 100% sure this should be sec-high anymore.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

  No, I don't think so.

Which older supported branches are affected by this flaw?

  The event in question is dispatched since Firefox 59, so I guess that also includes the most recent ESR.

If not all supported branches, which bug introduced the flaw?

  Bug 1399242.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

  No, but it'd be pretty trivial to write them.

How likely is this patch to cause regressions; how much testing does it need?

  Very unlikely, and the automated tests should be fine.

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Hey smaug, given the above, do you still think this should be sec-high? The event that's being kicked off is from our DevTools code, and I'm not 100% sure how that could put our users in any kind of danger.

Comment 18

[Al Billings [:abillings - ex-MoCo]]

Dan indicated in IRC that he thought this is more of a sec-moderate. I'm setting it to such and clearing the sec-approval? for it (since moderates don't need sec-approval to go in).

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #17)
> Hey smaug, given the above, do you still think this should be sec-high? The
> event that's being kicked off is from our DevTools code, and I'm not 100%
> sure how that could put our users in any kind of danger.

Depends on what other things happen when something handles the event. If nothing too bad, then this isn't sec-high.

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f3b19cd4415a382a94f77763164af233ded6a1ad
https://hg.mozilla.org/mozilla-central/rev/f3b19cd4415a

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Please request Beta & ESR60 approval on this when you get a chance.

Comment 22

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 9007866 [details]
Bug 1482402 - Dispatch DevTools toolbox visibilitychange event on the iframe document instead of the iframe window. r?gl

Approval Request Comment
[Feature/Bug causing the regression]:

Bug 1399242

[User impact if declined]:

It's possible for DevTools to fire an event at an "unsafe" time in our browser code. We're not sure if it's exploitable or not, but we're just being cautious.

[Is this code covered by automated tests?]:

The DevTools code is, yes.

[Has the fix been verified in Nightly?]:

The fix was verified after we landed bug 1451108 on Nightly, which causes our debug builds to crash when we get into this condition.

[Needs manual test from QE? If yes, steps to reproduce]: 

N/A

[List of other uplifts needed for the feature/fix]:

None.

[Is the change risky?]:

No.

[Why is the change risky/not risky?]:

We're changing the dispatching element of a visibilitychange event from a window to a document (which is where visibilitychange events should be dispatched from anyway)

[String changes made/needed]:

None.

Comment 23

[Mike Conley (:mconley) (:⚙️)]

Is this needed on ESR, since it's sec-moderate?

Comment 24

[Ryan VanderMeulen [:RyanVM]]

If the odds of being exploitable are low, we could probably just let it ride w/ Fx63. Should we revisit the sec rating (maybe sec-audit)?

Comment 25

[Mike Conley (:mconley) (:⚙️)]

Yeah, sounds good. I don't see how this could be used against the user, but I'll let people who've seen truly evil stuff evaluate.

Comment 26

[Pascal Chevrel:pascalc (PTO until April 26)]

Shouldn't this bug remain open now that the sec-audit keyword is set?

Comment 27

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9007866 [details]
Bug 1482402 - Dispatch DevTools toolbox visibilitychange event on the iframe document instead of the iframe window. r?gl

Uplift approved for 63 beta 8, thanks.

Comment 28

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/3ac8f0aae9c2

Comment 29

[Mike Conley (:mconley) (:⚙️)]

(In reply to Pascal Chevrel:pascalc from comment #26)
> Shouldn't this bug remain open now that the sec-audit keyword is set?

Sure, let's re-open it.

Comment 30

[:Gijs (he/him)]

This is showing up in my fx::general triage query. Mike, how should this be prioritized? Is there work left to do here? Is it urgent?

Comment 31

[Mike Conley (:mconley) (:⚙️)]

(In reply to :Gijs (he/him) from comment #30)
> This is showing up in my fx::general triage query. Mike, how should this be
> prioritized? Is there work left to do here? Is it urgent?

This is a sec-audit bug, so I think we're waiting for our security folks to tell us how exploitable this is, to see if we need to backport it to ESR.

So the urgency is unknown. The work we're waiting on is from the security team.

Comment 32

[Alex Gaynor [:Alex_Gaynor]]

I'm not sure I understand the state of this bug. There was a bug, and it's been fixed, as I understand it. Is that wrong? What are auditing for, more instance of this sort of bug, or assessing the severity?

Comment 33

[Mike Conley (:mconley) (:⚙️)]

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #32)
> I'm not sure I understand the state of this bug. There was a bug, and it's
> been fixed, as I understand it. Is that wrong?

You're right. There was a bug, and it's been fixed, but we're unsure of the severity, so we don't know if it needs to be backported to ESR. 

So yes, we're hoping for a severity audit to inform whether or not a backport is necessary, and whether this should ride into the next ESR point release.

Comment 34

[:Gijs (he/him)]

Is this ultimately just about devtools, then, or is there wider impact that we're still hoping to address in this bug? If the former, can you move this to devtools, and perhaps find someone to take point on the severity audit?

(Off-hand, it'd seem best to close this as there was a patch on the bug that has landed, and any follow-up could happen somewhere else - though I'm aware comment #29 explicitly reopened so I won't close this myself.)

Comment 35

[Mike Conley (:mconley) (:⚙️)]

(In reply to :Gijs (he/him) from comment #34)
> Is this ultimately just about devtools, then, or is there wider impact that
> we're still hoping to address in this bug? If the former, can you move this
> to devtools, and perhaps find someone to take point on the severity audit?

Sure, let's move this to DevTools. Since it was the only violator that caused the assertion to trip, I guess it's in their court.
 
> (Off-hand, it'd seem best to close this as there was a patch on the bug that
> has landed, and any follow-up could happen somewhere else - though I'm aware
> comment #29 explicitly reopened so I won't close this myself.)

Right - I reopened it at pascalec's request. I'll needinfo him if there's something else we need to do here.

Comment 36

[Pascal Chevrel:pascalc (PTO until April 26)]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #35)
> 
> Right - I reopened it at pascalec's request. I'll needinfo him if there's
> something else we need to do here.

I would expect somebody in the security team to have a look and close the bug or open a follow up if needed, which is my understanding of the sec-audit keyword per its definition (https://bugzilla.mozilla.org/describekeywords.cgi#sec-audit).

Al should we close the bug now that the patch has landed?

Comment 37

[Al Billings [:abillings - ex-MoCo]]

(In reply to Pascal Chevrel:pascalc from comment #36)

> Al should we close the bug now that the patch has landed?

Yes, that's fine.