1541557 - Support SpecialPowers in oop iframes

Status: RESOLVED FIXED

(Testing :: Mochitest, task, P2)

Description

[Nika Layzell [:nika] (ni? for response)]

Currently SpecialPowers doesn't function in out-of-process iframes under fission. The object is currently loaded using the old frameScript and MessageManager APIs, which are not supported in a post-fission world. This means that in out-of-process iframes, there is no SpecialPowers object available.

The SpecialPowers implementation system will need to be rewritten/updated, likely to use a JSWindowActor-based implementation.

Comment 1

[Neha Kochar [:neha]]

Discussed offline with Kris, and assigning this to him.

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1541557: Part 1 - Use correct globals for JSWindowActors not in the shared JSM global. r=nika

The current JSWindowActor code assumes that all actors will be created in the
shared JSM global. This has a few problems:

1. For actors in other scopes, it enters the wrong compartment before decoding
   messages, which leads to a compartment checker error when trying to call
   message handlers. That could be fixed by just wrapping the result for the
   caller, but that would lead to other problems. Aside from the efficiency
   concerns of having to deal with cross-compartment wrappers, SpecialPowers in
   particular would not be able to pass the resulting objects to unprivileged
   scopes, since only SpecialPowers compartments have permissive CCWs enabled.

2. It also leads to the prototype objects for the actor instances being
   created in the shared JSM scope, even when the actors themselves are defined
   in other compartments. Again, aside from CCW efficiency issues, this prevents
   the SpecialPowers instances from being accessed by the unprivileged scopes
   that they're exposed to, since the prototype objects live in privileged scopes
   which don't have permissive CCWs enabled.

This patch changes child actors to always create their objects in the global
of their constructors.

The parent objects are still created in the shared JSM global, but they now
wrap values for the appropriate compartment before trying to call message
handlers.

Comment 3

[Kris Maglione [:kmag]]

Attachment: Bug 1541557: Part 2 - Stop sending uninitialized StructuredCloneData objects. r=nika

There are a couple of places where JSWindowActor currently sends uninitialized
StructuredCloneData objects in places where it wants the message data to be
undefined. The problem with this is that it causes an error when trying to
read the data, which leads to odd behavior like sendQuery promises never
being settled if the message handler throws, or sendAsyncMessage and
sendQuery failing to decode the message if the caller doesn't pass a second
argument.

The errors reported for these problems also have no context, which means it's
very hard for a developer to figure out the source of the problem. And, to
make matters worse, the errors look the same as structured clone encoding
errors, so there isn't even the clue that the error is happening on decode.

This patch updates the offending code to always explicitly initialize the
structured clone data with undefined when that's what it wants, and adds
assertions to make it more obvious where the decode errors are actually
happening.

Comment 4

[Kris Maglione [:kmag]]

Attachment: Bug 1541557: Part 3 - Update callers of sync SpecialPowers functions to await the return value. r=nika

When we migrate SpecialPowers to a JSWindowActor, it will no longer be able to
use synchronous IPC messaging, which means that its current synchronous APIs
will have to become asynchronous.

This patch doesn't change the behavior of those functions, but it does change
their callers to await their return values rather than using them directly.
This pattern will work the same whether the functions return a promise or a
plain value, which simplifies the migration.

Comment 5

[Kris Maglione [:kmag]]

Attachment: Bug 1541557: Part 4 - Stop relying on synchronous preference getters/setters. r=nika

The SpecialPowers setPref/getPref APIs currently use synchronous messaging
to set and get preference values from the parent process. Aside from directly
affecting callers of those APIs, it also affects callers of pushPrefEnv,
which is meant to be asynchronous, but is in practice usually synchronous due
to the synchronous messaging it uses.

This patch updates the getPref APIs to use the in-process preference service
(which most callers are expecting anyway), and also updates the callers of
the setPref and pushPrefEnv APIs to await the result if they're relying on it
taking effect immediately.

Unfortunately, there are some corner cases in tests that appear to only work
because of the quirks of the current sync messaging approach. The synchronous
setPref APIs, for instance, trigger preference changes in the parent
instantly, but don't update the values in the child until we've returned to
the event loop and had a chance to process the notifications from the parent.
The differnce in timing leads some tests to fail in strange ways, which this
patch works around by just adding timeouts.

There should be follow-ups for test owners to fix the flakiness.

Comment 6

[Kris Maglione [:kmag]]

Attachment: Bug 1541557: Part 5 - Update callers of ChromeScript.sendSyncMessage to use sendQuery instead. r=nika

Since JSWindowActors don't have direct access to synchronous messaging,
ChromeScript callers are going to need to migrate to asynchronous messaging
and queries instead.

Since there's no comparable API to sendQuery for frame message managers, this
patch adds a stub that uses synchronous messaging, but makes the API appear
asynchronous, and migrates callers to use it instead of direct synchronous
messaging. This will be replaced with a true synchronous API in the actor
migration.

Fortunately, most of the time, this actually leads to simpler code. The
sendQuery API doesn't have the odd return value semantics of
sendSyncMessage, and can usually just be used as a drop-in replacement. Many
of the sendSyncMessage callers don't actually use the result, and can just
be changed to sendAsyncMessage. And many of the existing async messaging
users can be changed to just use sendQuery rather than sending messages and
adding response listeners.

However, the APZ code is an exception. It relies on intricate properties of
the event loop, and doesn't have an easy way to slot in promise handlers, so I
migrated it to using sync messaging via process message managers instead.

Comment 7

[Kris Maglione [:kmag]]

Attachment: Bug 1541557: Part 6 - Read scripts for loadChromeScript in child process rather than parent. r=nika

loadChromeScript is often called with http: URLs pointing to mochitest
resources, which need to be read synchronously before they can be executed.
Unfortunately, while the API for this pretends to be synchronous, it really
spins the event loop underneath.

When we attempt to use it in the parent, that means that we spin the event
loop and process messages from the child before the script has been executed.
And since those messages often contain messages intended for the chrome
script, that causes problems.

When the chrome script APIs use sync messaging, this doesn't matter much,
since the loadChromeScript call blocks the caller until the message handler
in the parent returns. When it uses async messaging, though, we have no such
luck, and the messages intended for the script get sent to the parent
immediately.

Loading the script contents in the child solves this problem, since it
reliably blocks the child callers until the script contents are ready, and
doesn't give them a chance to try to send messages to the script while it's
still being read.

Comment 8

[Kris Maglione [:kmag]]

Attachment: Bug 1541557: Part 7 - Convert SpecialPowers to use JSWindowActors rather than framescripts. r=nika

Comment 9

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/61fa2745733f3631488a3ecccc144823683b7b6d
Bug 1541557: Part 1 - Use correct globals for JSWindowActors not in the shared JSM global. r=nika

https://hg.mozilla.org/integration/mozilla-inbound/rev/158a4000c44b9b17a7935340db79431d544fb556
Bug 1541557: Part 2 - Stop sending uninitialized StructuredCloneData objects. r=nika

https://hg.mozilla.org/integration/mozilla-inbound/rev/b4ed40bea2698802ef562a0931c0b560737fb89d
Bug 1541557: Part 3 - Update callers of sync SpecialPowers functions to await the return value. r=nika

https://hg.mozilla.org/integration/mozilla-inbound/rev/189dc8a359815e059a4a217f788d183260bb2bfe
Bug 1541557: Part 4 - Stop relying on synchronous preference getters/setters. r=nika

https://hg.mozilla.org/integration/mozilla-inbound/rev/75ebd6fce136ab3bd0e591c2b8b2d06d3b5bf923
Bug 1541557: Part 5 - Update callers of ChromeScript.sendSyncMessage to use sendQuery instead. r=nika

https://hg.mozilla.org/integration/mozilla-inbound/rev/c2697f04d38cf0b01b1f3e227910ab5890926a33
Bug 1541557: Part 6 - Read scripts for loadChromeScript in child process rather than parent. r=nika

https://hg.mozilla.org/integration/mozilla-inbound/rev/46ff845a7b0cdabf640bb2e3c783735ab68b7cd1
Bug 1541557: Part 7 - Convert SpecialPowers to use JSWindowActors rather than framescripts. r=nika

Comment 10

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8802daac6779f4707edb59d7d578735b40870716
Bug 1541557: Follow-up: Fix straggler test that relies on sync SpecialPowers pref setters. r=bustage CLOSED TREE

https://hg.mozilla.org/integration/mozilla-inbound/rev/d972bbdfe03933e00f4a7ecbea2dc1f8ac85fd12
Bug 1541557: Follow-up: Fix intermittent error from shutdown race in WebExtension tests. r=bustage CLOSED TREE

Comment 11

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a7eab5ca061da40d0a4e882064bc90ec257576ac
Bug 1541557: Follow-up: Fix intermittent error in passwordmgr tests. r=bustage

Comment 12

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a4fe24a016c5530f94892c707a8a08343a4695a
Bug 1541557: Follow-up: Increase timeout after pref-change a bit more. r=bustage

Comment 13

[Florin Strugariu [:Bebe]]

== Change summary for alert #21726 (as of Wed, 03 Jul 2019 11:17:40 GMT) ==

Improvements:

24% build times linux32-shippable opt instrumented taskcluster-m5.4xlarge 1,229.23 -> 936.50

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=21726

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/255735c1ff63
Part 1 - Use correct globals for JSWindowActors not in the shared JSM global. r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/6680278c231b
Part 2 - Stop sending uninitialized StructuredCloneData objects. r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe88b250e418
Part 3 - Update callers of sync SpecialPowers functions to await the return value. r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/1025eeef0954
Part 4 - Stop relying on synchronous preference getters/setters. r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/f808ec45ff9c
Part 5 - Update callers of ChromeScript.sendSyncMessage to use sendQuery instead. r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/054a0b7aa81d
Part 6 - Read scripts for loadChromeScript in child process rather than parent. r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2c260cae4b5
Part 7 - Convert SpecialPowers to use JSWindowActors rather than framescripts. r=nika

Comment 15

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/210d6d52e8b0677448cb8e84befeb637ca87eb29
Bug 1541557: Follow-up: Temporarily disable test_font_whitelist in debug/ASan for frequent failures. r=bustage

Comment 16

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/255735c1ff63
https://hg.mozilla.org/mozilla-central/rev/6680278c231b
https://hg.mozilla.org/mozilla-central/rev/fe88b250e418
https://hg.mozilla.org/mozilla-central/rev/1025eeef0954
https://hg.mozilla.org/mozilla-central/rev/f808ec45ff9c
https://hg.mozilla.org/mozilla-central/rev/054a0b7aa81d
https://hg.mozilla.org/mozilla-central/rev/f2c260cae4b5
https://hg.mozilla.org/mozilla-central/rev/210d6d52e8b0

Comment 17

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

Backed out 34 changesets (bug 1561150, bug 1541557, bug 1561724, bug 1561999, bug 1558298, bug 1561061, bug 1532795, bug 1560400, bug 1561122) for beta simulation failures (bug 1563905, bug 1564001):
https://hg.mozilla.org/mozilla-central/rev/7e6657f88b7694ecd841088963ff71d767e4ec22

Comment 18

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/a636725ad217
Backout of bug 1541557 - follow-up: add back underscore. a=backout CLOSED TREE

Comment 19

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/5b91c8869f42
Backout of Bug 1541557 - follow-up: correct function name. a=backout CLOSED TREE

Comment 20

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/e596664275d5
Backed out 3 changesets for failures in SpecialPowersObserverAPI.js. a=backout CLOSED TREE

Comment 21

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

Relanded (backout got backed out):
https://hg.mozilla.org/mozilla-central/rev/e596664275d5e3e2fdcb7fa8d1447289f99269c3