1447327 - Assertion failure: objectFromIncumbentGlobal, at /builds/worker/workspace/build/src/js/src/builtin/Promise.cpp:756

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect, P1)

Description

[Jason Kratzer [:jkratzer]]

Attachment: trigger_part_1.html

Testcase found while fuzzing mozilla-central rev 827c686c5709.  Testcase must be served via local webserver and may not trigger for several minutes.

rax = 0x0000000000000000   rdx = 0x0000000000000000
rcx = 0x00007ff52c86b2dd   rbx = 0x00007ffcaf233748
rsi = 0x00007ff52cb3a770   rdi = 0x00007ff52cb39540
rbp = 0x00007ffcaf2338f0   rsp = 0x00007ffcaf233700
r8 = 0x00007ff52cb3a770    r9 = 0x00007ff52dc11740
r10 = 0x0000000000000000   r11 = 0x0000000000000000
r12 = 0x00007ffcaf233880   r13 = 0x00007ffcaf233780
r14 = 0x00007ffcaf233840   r15 = 0x00007ffcaf233860
rip = 0x00007ff51e87dd88
OS|Linux|0.0.0 Linux 4.4.0-103-generic #126-Ubuntu SMP Mon Dec 4 16:23:28 UTC 2017 x86_64
CPU|amd64|family 6 model 60 stepping 3|1
GPU|||
Crash|SIGSEGV|0x0|0
0|0|libxul.so|EnqueuePromiseReactionJob|hg:hg.mozilla.org/mozilla-central:js/src/builtin/Promise.cpp:827c686c570935483c3ad8022aa92b9e005574c3|756|0x18
0|1|libxul.so|ResolvePromise|hg:hg.mozilla.org/mozilla-central:js/src/builtin/Promise.cpp:827c686c570935483c3ad8022aa92b9e005574c3|1058|0x5
0|2|libxul.so|FulfillMaybeWrappedPromise|hg:hg.mozilla.org/mozilla-central:js/src/builtin/Promise.cpp:827c686c570935483c3ad8022aa92b9e005574c3|837|0xa
0|3|libxul.so|ResolvePromiseInternal|hg:hg.mozilla.org/mozilla-central:js/src/builtin/Promise.cpp:827c686c570935483c3ad8022aa92b9e005574c3|597|0x12
0|4|libxul.so|js::PromiseObject::resolve|hg:hg.mozilla.org/mozilla-central:js/src/builtin/Promise.cpp:827c686c570935483c3ad8022aa92b9e005574c3|3433|0x12
0|5|libxul.so|ResolveOrRejectPromise|hg:hg.mozilla.org/mozilla-central:js/src/jsapi.cpp:827c686c570935483c3ad8022aa92b9e005574c3|5229|0x5
0|6|libxul.so|mozilla::dom::Promise::MaybeResolve|hg:hg.mozilla.org/mozilla-central:dom/promise/Promise.cpp:827c686c570935483c3ad8022aa92b9e005574c3|250|0x8
0|7|libxul.so|mozilla::dom::Promise::MaybeSomething<const RefPtr<mozilla::dom::ImageBitmap> >|hg:hg.mozilla.org/mozilla-central:dom/promise/Promise.h:827c686c570935483c3ad8022aa92b9e005574c3|194|0x7
0|8|libxul.so|mozilla::dom::FulfillImageBitmapPromise::DoFulfillImageBitmapPromise|hg:hg.mozilla.org/mozilla-central:dom/promise/Promise.h:827c686c570935483c3ad8022aa92b9e005574c3|76|0x11
0|9|libxul.so|mozilla::dom::FulfillImageBitmapPromiseTask::Run|hg:hg.mozilla.org/mozilla-central:dom/canvas/ImageBitmap.cpp:827c686c570935483c3ad8022aa92b9e005574c3|1186|0x5
0|10|libxul.so|nsThread::ProcessNextEvent|hg:hg.mozilla.org/mozilla-central:xpcom/threads/nsThread.cpp:827c686c570935483c3ad8022aa92b9e005574c3|1096|0x15
0|11|libxul.so|NS_ProcessNextEvent|hg:hg.mozilla.org/mozilla-central:xpcom/threads/nsThreadUtils.cpp:827c686c570935483c3ad8022aa92b9e005574c3|517|0x11
0|12|libxul.so|mozilla::ipc::MessagePump::Run|hg:hg.mozilla.org/mozilla-central:ipc/glue/MessagePump.cpp:827c686c570935483c3ad8022aa92b9e005574c3|97|0xa
0|13|libxul.so|MessageLoop::RunInternal|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_loop.cc:827c686c570935483c3ad8022aa92b9e005574c3|326|0x17
0|14|libxul.so|MessageLoop::Run|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_loop.cc:827c686c570935483c3ad8022aa92b9e005574c3|319|0x8
0|15|libxul.so|nsBaseAppShell::Run|hg:hg.mozilla.org/mozilla-central:widget/nsBaseAppShell.cpp:827c686c570935483c3ad8022aa92b9e005574c3|157|0xd
0|16|libxul.so|XRE_RunAppShell|hg:hg.mozilla.org/mozilla-central:toolkit/xre/nsEmbedFunctions.cpp:827c686c570935483c3ad8022aa92b9e005574c3|893|0x11
0|17|libxul.so|mozilla::ipc::MessagePumpForChildProcess::Run|hg:hg.mozilla.org/mozilla-central:ipc/glue/MessagePump.cpp:827c686c570935483c3ad8022aa92b9e005574c3|269|0x5
0|18|libxul.so|MessageLoop::RunInternal|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_loop.cc:827c686c570935483c3ad8022aa92b9e005574c3|326|0x17
0|19|libxul.so|MessageLoop::Run|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_loop.cc:827c686c570935483c3ad8022aa92b9e005574c3|319|0x8
0|20|libxul.so|XRE_InitChildProcess|hg:hg.mozilla.org/mozilla-central:toolkit/xre/nsEmbedFunctions.cpp:827c686c570935483c3ad8022aa92b9e005574c3|719|0x8
0|21|firefox|content_process_main|hg:hg.mozilla.org/mozilla-central:ipc/contentproc/plugin-container.cpp:827c686c570935483c3ad8022aa92b9e005574c3|50|0x14
0|22|firefox|main|hg:hg.mozilla.org/mozilla-central:browser/app/nsBrowserApp.cpp:827c686c570935483c3ad8022aa92b9e005574c3|280|0x11
0|23|libc-2.23.so||||0x20830
0|24|firefox|MOZ_ReportAssertionFailure|hg:hg.mozilla.org/mozilla-central:mfbt/Assertions.h:827c686c570935483c3ad8022aa92b9e005574c3|164|0x5

Comment 1

[Jason Kratzer [:jkratzer]]

Attachment: trigger_part_2.html

Comment 2

[Ryan Hunt [:rhunt]]

From the stack here it looks like this is JS or DOM related, moving to JS component as a guess because it's the final stack frame here.

Comment 3

[Jason Orendorff [:jorendorff]]

André, would you mind taking a look?

Comment 4

[André Bargull [:anba]]

CheckedUnwrap at <https://searchfox.org/mozilla-central/rev/49cc27555d5b7ed2eb06baf156693dd578daa06f/js/src/builtin/Promise.cpp#755> returns nullptr because |reaction->incumbentGlobalObject()| is wrapper with a security policy (cf. UnwrapOneChecked):
---
(gdb) p IsWrapper(reaction->incumbentGlobalObject())                                                                                                                                                        
$7 = true
(gdb) p Wrapper::wrapperHandler(reaction->incumbentGlobalObject())
$8 = (const xpc::FilteringWrapper<js::SecurityWrapper<js::CrossCompartmentWrapper>, xpc::Opaque> *) 0x7fffec78b340 <xpc::FilteringWrapper<js::SecurityWrapper<js::CrossCompartmentWrapper>, xpc::Opaque>::singleton>
(gdb) p Wrapper::wrapperHandler(reaction->incumbentGlobalObject())->hasSecurityPolicy()
$9 = true
---

I don't really know the relevant WHATWG/DOM/HTML spec(s), so I can't tell if we can simply call ReportAccessDenied() here or we need to handle this case differently. :till or :bz should know more.

Comment 5

[Boris Zbarsky [:bzbarsky]]

It sure sounds like in addition to whatever is going on with the JS side (which is definitely broken and needs to be fixed; see below) there's misuse of some sort on the DOM side...

Specifically, DoFulfillImageBitmapPromise does:

    mPromise->MaybeResolve(mImageBitmap);

That will call JS::ResolvePromise.  This can happen with a security wrapper; if the JS code can't handle that, it's broken.  How it handles that... I don't know; there are no spec provisions for this, because this is all part of the ES spec and that spec pretends there are no different security origins.  Ideally, resolving a Promise with a cross-origin object would Just Work, because people want to do that for cross-origin windows.  And it does work for scripted Promise uses, so how is this case different?

For the rest, which globals mPromise and mImageBitmap are associated with would ideally be defined by a spec.  Unfortunately, https://html.spec.whatwg.org/multipage/imagebitmap-and-animations.html#dom-createimagebitmap doesn't actually define the behavior... but it would be logical to assume that the same global should be used for both the promise and the image bitmap.  And that certainly seems to be the case in our code: ImageBitmap::Create uses the same nsIGlobalObject for both.

So the first question is: why is there a security wrapper here at all?

Comment 6

[André Bargull [:anba]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #5)
> So the first question is: why is there a security wrapper here at all?

The `document.domain = document.domain;` assignment seems to trigger the creation of the security wrapper. 

For this simplified test case, I get `SecurityError: Permission denied to access property "createImageBitmap" on cross-origin object` when I click on the "Create without PromiseReaction" button multiple times. Does that mean by extension that it's also okay to call ReportAccessDenied() in EnqueuePromiseReactionJob() when `reaction->incumbentGlobalObject()` is a wrapper with a security policy?

trigger_part_1.html
---
<html>
  <body>
    <iframe id="iwin" src="trigger_part_2.html"></iframe>
    <canvas id="c"></canvas>
    <button onclick="onClick(true)">Create with PromiseReaction</button>
    <button onclick="onClick(false)">Create without PromiseReaction</button>
    <script>
      function onClick(createPromiseReaction) {
        var ctxt = document.getElementById("c").getContext('2d');
        var iframeWin = document.getElementById("iwin").contentWindow;
        var p = iframeWin.createImageBitmap(ctxt);

        if (createPromiseReaction)
            p.then();

        document.domain = document.domain;
      }
    </script>
  </body>
</html>
---

trigger_part_2.html
---
<html>
</html>
---

Comment 7

[Boris Zbarsky [:bzbarsky]]

> The `document.domain = document.domain;` assignment seems to trigger the creation of the security wrapper. 

Aha.  I had missed that in the testcase.

> Does that mean by extension that it's also okay to call ReportAccessDenied()
> in EnqueuePromiseReactionJob() when `reaction->incumbentGlobalObject()` is a
> wrapper with a security policy?

It depends on what reaction->incumbentGlobalObject() is and what it means for it to have a security policy....

What is the actual ordering of events here?  Is it basically something like:

1)  Promise is created in global A.
2)  ImageBitmap is created in global A.
3)  Promise has then() called on it from same-origin global B
    (so the incumbent global of the call is B).
4)  Before the then() callback is invoked, B stops being same-origin with A.
5)  When we go to invoke the then() callback, the incumbent global we want to use
    is no longer same-origin with us.

?

Comment 8

[Boris Zbarsky [:bzbarsky]]

And in particular, do we need to end up resolving the promise after the document.domain set?  I tried just now to reproduce this problem with just Promise, without using createImageBitmap, but haven't managed to so far.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Ah, I found a pure-Promise way to reproduce:

trigger_part_1.html:

  <iframe src="trigger_part_2.html"></iframe>
  <script>
  onload = function() {
    var p = frames[0].getPromise();
    p.then((val) => console.log(val));
    document.domain = document.domain;
  }
  </script>

trigger_part_2.html:

  <script>
  function getPromise() {
    var p = new Promise((r) => {
      setTimeout(() => r(5), 1000);
    });
    return p;
  }
  </script>

OK.  So in this situation, doing some sort of security exception doesn't seem unreasonable.  What will the behavior be in practice if we do that?  The then callback is never called and we report an error to console?  What about other callbacks on the same promise, ones that have accessible global objects an whatnot?

Comment 10

[Intermittent Failures Robot]

1 failures in 4439 pushes (0.0 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• mozilla-inbound: 1

Platform breakdown:

• macosx1014-64: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1447327&startday=2019-07-29&endday=2019-08-04&tree=all

Comment 11

[Jason Orendorff [:jorendorff]]

Jeff, this is a bug that we think has caused crashes (bug 1552099) with a reproducible test case. Please fix it.

Comment 12

[Jeff Walden [:Waldo]]

The simplified testcase from comment 9 now reports JavaScript error: file:///home/jwalden/moz/inflight/1447327/test.html, line 4: SecurityError: Permission denied to access property "getPromise" on cross-origin object for me when I try to run it locally, from a file:// URL. That seems like the desired behavior, maybe?

Alternatively, if I run the tests from an httpd.js quick stood-up web server, nothing crashes, 5 is logged to the console, and everything seems hunky-dory.

It doesn't appear to me there's actually a problem here any more, and I'm inclined to resolve this WORKSFORME, unless someone can show there's still a problem.

Comment 13

[Jeff Walden [:Waldo]]

Attachment: supporting.html

Comment 14

[Jeff Walden [:Waldo]]

Attachment: test.html

Comment 15

[Boris Zbarsky [:bzbarsky]]

That seems like the desired behavior, maybe?

Yes, for file://. This testcase needs to be run over http.

I am hopeful that the incumbent fixes we made to promises at some point fix this.

Comment 16

[Jeff Walden [:Waldo]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #15)

That seems like the desired behavior, maybe?

Yes, for file://. This testcase needs to be run over http.

Sure, and I did that (the "Alternatively" bit). Okay to close then?

Comment 17

[Boris Zbarsky [:bzbarsky]]

Seems so...

Comment 18

[Nicolas B. Pierron [:nbp]]

Per comment 12, comment 16 and comment 17.
Closing as works-for-me.

Comment 19

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug is a regression, but please revert this change in case of error.