Support SpecialPowers in oop iframes
Categories
(Testing :: Mochitest, task, P2)
Tracking
(Fission Milestone:M4, firefox69 fixed)
| Tracking | Status | |
|---|---|---|
| firefox69 | --- | fixed |
People
(Reporter: nika, Assigned: kmag)
References
(Regressed 2 open bugs)
Details
Attachments
(7 files)
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review |
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•6 years ago
|
||
Discussed offline with Kris, and assigning this to him.
|
Assignee | |
Comment 2•6 years ago
|
||
The current JSWindowActor code assumes that all actors will be created in the
shared JSM global. This has a few problems:
-
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. -
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.
|
|
Assignee | |
Comment 3•6 years ago
|
||
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.
|
|
Assignee | |
Comment 4•6 years ago
|
||
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.
|
|
Assignee | |
Comment 5•6 years ago
|
||
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.
|
|
Assignee | |
Comment 6•6 years ago
|
||
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.
|
|
Assignee | |
Comment 7•6 years ago
|
||
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.
|
|
Assignee | |
Comment 8•6 years ago
|
||
|
|
Assignee | |
Comment 9•6 years ago
|
||
|
|
Assignee | |
Comment 10•6 years ago
|
||
|
|
Assignee | |
Comment 11•6 years ago
|
||
|
|
Assignee | |
Comment 12•6 years ago
|
||
|
||
Comment 13•6 years ago
|
||
== 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•6 years ago
|
||
|
|
Assignee | |
Comment 15•6 years ago
|
||
|
||
Comment 16•6 years ago
|
||
| bugherder | ||
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•6 years ago
|
||
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•6 years ago
|
||
|
|
||
Comment 19•6 years ago
|
||
|
|
||
Comment 20•6 years ago
|
||
|
|
||
Comment 21•6 years ago
|
||
Relanded (backout got backed out):
https://hg.mozilla.org/mozilla-central/rev/e596664275d5e3e2fdcb7fa8d1447289f99269c3
|
|
||
Updated•6 years ago
|
Description
•