1723124 - Promise reaction job is not called when it is bound with Function.prototype.bind from iframe with discarded browsing context

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[roland]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/92.0.4515.107 Safari/537.36

Steps to reproduce:

1. Open https://hermanitos.ch/index.php/shop
2. Open developer console
3. Paste and run the following code:
   (new Promise(function (resolve) {
   resolve();
   }))
   .then(function () {
   console.log('Hello')
   })
   .then(function () {
   console.log('World')
   }.bind(this));

Actual results:

Prints on the console:
Hello

Expected results:

Prints on the console:
Hello
World

Comment 1

[Tom S. (please needinfo tschuster)]

Very strange. I can reproduce this issue on the site you mentioned, but not on e.g. http://example.org. The Promise stays pending. Arai do you have an idea? I thought maybe then is overwritten or something, but I doesn't look like it.

Comment 2

[roland]

I tried to debug it myself and all core things what I checked were [native code]

Promise.toString()

Function.bind.toString()

(function () {
console.log('World')
}).bind.toString()

(new Promise(function (resolve) {})).then.toString()

(new Promise(function (resolve) {
resolve();
})).then(function(){}).then.toString()

This issue affects all sites which made with Ecwid: https://www.ecwid.com
So it might be an error in their code, but it looks like they triggered some kind of browser bug.

Comment 3

[Tooru Fujisawa [:arai]]

it happens if a page loads https://app.ecwid.com/script.js?6848023
will investigate more.

Comment 4

[roland]

Attachment: test.html

Attached a reduced test.

I was able to narrow it down, but stuck. I think something in this injected script cause this issue:
https://d2scn539ulxr09.cloudfront.net/static/br/2021-30677-g515270bf75f/F57A9FB1FC384BAF8CF3B63EFDEAE420.cache.js

Comment 5

[Alice0775 White]

Regression window(using test.html ):
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ae502e2271f2464d92ef398d7c8c43f835865bd6&tochange=efcf012e6cfc24413bea5078650e4528be361205

Comment 6

[Tooru Fujisawa [:arai]]

there's the following code (prettified), in https://d2scn539ulxr09.cloudfront.net/static/br/2021-30677-g515270bf75f/A66F35FBA011F122D2A83F7B91CC58EA.cache.js

var frame = document.createElement('iframe');
frame.src = 'about:blank';
frame.style.display='none';
document.body.appendChild(frame);
Function.prototype.bind = frame.contentWindow.Function.prototype.bind;
frame.remove();

that seems to be related, given it's about Function.prototype.bind

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: minimal testcase

here's minimal testcase

Comment 8

[Tooru Fujisawa [:arai]]

Attachment: minimal testcase

fixed some typo
no change in the logic

Comment 9

[Tooru Fujisawa [:arai]]

it's a bug when creating a promise reaction job.

the spec requires using GetFunctionRealm, that handles bound function.

https://tc39.es/ecma262/#sec-newpromisereactionjob

27.2.2.1 NewPromiseReactionJob ( reaction, argument )
...
3. If reaction.[[Handler]] is not empty, then
  a. Let getHandlerRealmResult be GetFunctionRealm(reaction.[[Handler]].[[Callback]]).
  b. If getHandlerRealmResult is a normal completion, set handlerRealm to getHandlerRealmResult.[[Value]].
  c. Else, set handlerRealm to the current Realm Record.
  d. NOTE: handlerRealm is never null unless the handler is undefined. When the handler is a revoked Proxy and no ECMAScript code runs, handlerRealm is used to create error objects.
4. Return the Record { [[Job]]: job, [[Realm]]: handlerRealm }.

https://tc39.es/ecma262/#sec-getfunctionrealm

7.3.24 GetFunctionRealm ( obj )
...
    3. If obj is a bound function exotic object, then
        a. Let target be obj.[[BoundTargetFunction]].
        b. Return ? GetFunctionRealm(target).

but we just use the function object's realm,
that's bind function's realm in this case.

https://searchfox.org/mozilla-central/rev/2aa97aea1085cf1363582725407c514833ad47e4/js/src/builtin/Promise.cpp#1191

  mozilla::Maybe<AutoRealm> ar2;
  if (handler.isObject()) {
...
    JSObject* handlerObj = UncheckedUnwrap(&handler.toObject());
    MOZ_ASSERT(handlerObj);
    ar2.emplace(cx, handlerObj);

So, in the spec, the reaction job is created with the top-level frame's realm,
but we create the reaction job with iframe's realm.

and HostEnqueuePromiseJob doesn't call the reaction job if the iframe is removed from the tree.

https://html.spec.whatwg.org/#hostenqueuepromisejob

Comment 10

[Tooru Fujisawa [:arai]]

Attachment: Bug 1723124 - Part 1: Use GetFunctionRealm when creating promise reaction/thenable jobs. r?jandem!

Creating reaction job should use GetFunctionRealm with unchecked unwrap,
to support promise across chrome/content compartments.

Creating thenable job should use GetFunctionRealm with checked unwrap,
given thenable job across chrome/content compartments isn't supposed to happen
in normal case.

Comment 11

[Tooru Fujisawa [:arai]]

Attachment: Bug 1723124 - Part 2: Add JS shell testing function for promise jobs. r?jandem!

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0fcc1b66665f
Part 1: Use GetFunctionRealm when creating promise reaction/thenable jobs. r=jandem
https://hg.mozilla.org/integration/autoland/rev/ec84082e8780
Part 2: Add JS shell testing function for promise jobs. r=jandem

Comment 13

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/0fcc1b66665f
https://hg.mozilla.org/mozilla-central/rev/ec84082e8780

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider uplifting to Beta?

Comment 15

[Tooru Fujisawa [:arai]]

Comment on attachment 9234811 [details]
Bug 1723124 - Part 1: Use GetFunctionRealm when creating promise reaction/thenable jobs. r?jandem!

Beta/Release Uplift Approval Request

• User impact if declined: Website breakage (very edge case tho), parity with other browsers.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): It changes the behavior around the security wrapper between privilege code and web content code, mostly hardening the boundary, but introduces extra paths than before.
• String changes made/needed:

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9234811 [details]
Bug 1723124 - Part 1: Use GetFunctionRealm when creating promise reaction/thenable jobs. r?jandem!

Approved for 92.0b3.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/423da1dcb0f4
https://hg.mozilla.org/releases/mozilla-beta/rev/ea90868b351c