Closed Bug 1526424 Opened 6 years ago Closed 6 years ago

Fix failures due to lack of support for the newCompartment option to newGlobal in jsreftests

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox67 --- fixed

People

(Reporter: bzbarsky, Assigned: anba)

References

Details

Attachments

(1 file, 2 obsolete files)

I'm going to need it to enable multiple-globals-per-compartment in Gecko.

I looked at this briefly today, and there are some complications:

  1. newGlobal() expects to synchronously return a global. The only way to synchronously create a global on the web is to create an initial about:blank, but that always inherits the origin of the creator, and hence in our compartment-per-origin world will be same-compartment.

  2. The requirement is that newGlobal() return a global that the caller can touch. Which means that in terms of web technologies, we'd need to start with a different-origin thing and then make it accessible (e.g. via document.domain). And the load would have to be async.

So sticking to web-exposed stuff and making this work doesn't really seem possible. Some options:

  • Add some sort of privileged-only way to tell an iframe to not share compartments with its parent (e.g. via attribute). It's not clear to me whether this harness runs with any interesting privileges that we could gate that one, and it would require special Gecko code to support this weird mode.

  • Just skip the tests that need newCompartment when running jsreftest.

Any other bright ideas?

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

  • Just skip the tests that need newCompartment when running jsreftest.

That makes sense for now I think, it's just one test? We could mark that one shell-only. Eventually maybe the proposed Realms API will help here or we could do something more involved.

Attached patch bug1526424.patch (obsolete) (deleted) — Splinter Review

Only these two TypedArray tests need to be modified. And because I've originally written them, I think it's only fair that I also update them to work with same-compartment globals. :-)

Drive-by fixes:

  • Remove dynamic check if newGlobal is present, because it is no longer needed (no longer shell-only) and other tests already require it to be present.
  • Replace dynamic check for Debugger with |skip-if| guard to ensure we're running in the shell.
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Attachment #9042908 - Flags: review?(jdemooij)
Attached patch bug1526424.patch (obsolete) (deleted) — Splinter Review

Updated patch to remove global. in typeof global.window test.

Attachment #9042908 - Attachment is obsolete: true
Attachment #9042908 - Flags: review?(jdemooij)
Attachment #9042934 - Flags: review?(jdemooij)
Comment on attachment 9042934 [details] [diff] [review] bug1526424.patch Review of attachment 9042934 [details] [diff] [review]: ----------------------------------------------------------------- Excellent. Thanks!
Attachment #9042934 - Flags: review?(jdemooij) → review+

Hmm, I probably should have written a patch which works with and without multiple globals per compartment. I'll update it later and request a new review.

Attached patch bug1526424.patch (deleted) — Splinter Review

Alternative patch which works in the browser runner with and without multiple globals per compartment. This allows to check in the patch in current central without waiting for the multiple globals per compartment work.

There is already another test which accepts a TypeError from either global, so let's do the same here, too.

Attachment #9042934 - Attachment is obsolete: true
Attachment #9043251 - Flags: review?(jdemooij)

I've already checked the new patch works with current central and with bz's patch queue for the multiple globals per compartment work.

Comment on attachment 9043251 [details] [diff] [review] bug1526424.patch Review of attachment 9043251 [details] [diff] [review]: ----------------------------------------------------------------- Thanks. Sorry I should have noticed this would fail when landed without the other compartment changes :)
Attachment #9043251 - Flags: review?(jdemooij) → review+
Keywords: checkin-needed

Pushed by ncsoregi@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/91023d410af1
Add fallback because browser "newGlobal()" doesn't support the "newCompartment" option. r=jandem

Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: