Perma W-fis(Wr) UnknownError: SecurityError: Permission denied to access property "windowUtils" on cross-origin object (flushWindow@chrome://marionette/content/actors/MarionetteReftestChild.jsm, line 176)
Categories
(Testing :: Marionette Client and Harness, defect, P3)
Tracking
(Fission Milestone:M7, firefox88 fixed)
Tracking | Status | |
---|---|---|
firefox88 | --- | fixed |
People
(Reporter: intermittent-bug-filer, Assigned: jdescottes)
References
(Blocks 1 open bug)
Details
(Keywords: intermittent-failure, regression, Whiteboard: [marionette-fission-reserve])
Attachments
(1 file)
(deleted),
text/x-phabricator-request
|
Details |
Filed by: hskupin [at] mozilla.com
Parsed log: https://treeherder.mozilla.org/logviewer.html#?job_id=262016165&repo=try
Full log: https://queue.taskcluster.net/v1/task/VaTdEsDSS_SlY9vkQb38mg/runs/0/artifacts/public/logs/live_backing.log
Reftest URL: https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/VaTdEsDSS_SlY9vkQb38mg/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1
Comment 1•5 years ago
|
||
With Fission enabled a lot of the web-platform-tests based reftests currently fail for taking screenshots when an OOP iframe is part of the testcase. The exception as thrown is:
[task 2019-08-16T14:26:38.850Z] 14:26:38 INFO - UnknownError: SecurityError: Permission denied to access property "windowUtils" on cross-origin object
[task 2019-08-16T14:26:38.851Z] 14:26:38 INFO - flushWindow@chrome://marionette/content/listener.js:1702:17
[task 2019-08-16T14:26:38.851Z] 14:26:38 INFO - flushWindow@chrome://marionette/content/listener.js:1720:18
[task 2019-08-16T14:26:38.851Z] 14:26:38 INFO - flushRendering@chrome://marionette/content/listener.js:1723:14
[task 2019-08-16T14:26:38.851Z] 14:26:38 INFO - maybeResolve@chrome://marionette/content/listener.js:1793:21
[task 2019-08-16T14:26:38.852Z] 14:26:38 INFO - reftestWait/<@chrome://marionette/content/listener.js:1808:17
[task 2019-08-16T14:26:38.852Z] 14:26:38 INFO - reftestWait@chrome://marionette/content/listener.js:1791:9
[task 2019-08-16T14:26:38.853Z] 14:26:38 INFO - async*dispatch/</req<@chrome://marionette/content/listener.js:525:17
[task 2019-08-16T14:26:38.853Z] 14:26:38 INFO - dispatch/<@chrome://marionette/content/listener.js:520:15
[task 2019-08-16T14:26:38.853Z] 14:26:38 INFO - MessageListener.receiveMessage*startListeners@chrome://marionette/content/listener.js:599:21
[task 2019-08-16T14:26:38.853Z] 14:26:38 INFO - registerSelf@chrome://marionette/content/listener.js:501:19
[task 2019-08-16T14:26:38.853Z] 14:26:38 INFO - @chrome://marionette/content/listener.js:1821:13
Here it's happening:
https://searchfox.org/mozilla-central/rev/3a61fb322f74a0396878468e50e4f4e97e369825/testing/marionette/listener.js#1704
When comparing the code with the original reftest harness I can see that it doesn't raise an exception as Marionette currently does, but only logs an error:
Is ignoring the failure ok, or should it really be taken into account? If the latter, how to best accomplish this? I cannot find any filed bug about the reftest case yet.
Kris and David, what's your take here?
Updated•5 years ago
|
Comment hidden (Intermittent Failures Robot) |
Comment 3•5 years ago
|
||
A few thoughts:
- it's not clear to me why that code is catching exceptions the way it is; it was added in 95da39224eab although the
catch
around thegetBoundingClientRect
call is much much older (and traces its history into another file). Catching lots of exceptions doesn't seem great... - as to what really should be happening here, I think Matt is probably a better person to answer (or delegate to others) how the reftest harness should be operating in a fission world where some of the iframes are in other processes.
Comment 4•5 years ago
|
||
Timothy is looking into making the reftest harness fission compatible.
I believe the current plan is to use JSWindowActors to run a frame script in each iframe process, and use that (and message passing from reftest.jsm/reftest-contest.js) to coordinate calling flush from all processes that might need to contribute to our rendering.
Comment 5•5 years ago
|
||
At this point, the bigger problem for these tests is that they still use canvases to capture screenshots, so regardless of whether we successfully flush layout for cross-process frames, they won't paint. So at least as far as that is concerned, I don't see any reason to skip any attempt to flush layout for remote frames until we at least support capturing them properly. In any case, I'll defer to Matt and Timothy on that.
That said, I'm also not entirely sure what the purpose of these layout flushes is. Given that I'd expect drawWindow
or drawSnapshot
to do any necessary layout flushes, I'm assuming it has more to do with the subsequent code which waits for an animation frame or MozAfterPaint
event, so this is more a question for someone who understands why those are there.
Comment 6•5 years ago
|
||
For the reftest harness (reftest.jsm), we're using canvas.drawWindow(USE_WIDGET_LAYERS) from the parent process, which does a readback of the compositor content, and will include any OOP content (from both content and iframe processes).
drawWindow does flush layout, but only the parent process, so we need to manually make sure that child processes are flushed, and have sent their content to the compositor (we use sync ipc via nsIDOMWindowUtils.updateLayerTree()).
That latter bit is the one that currently only happens for content processes, and needs to be expanded to include iframe processes when fission is enabled.
Comment 7•5 years ago
|
||
Tracking for Fission milestone M6 because we want to be able to run Marionette tests with Fission before enabling in Nightly.
Comment 8•5 years ago
|
||
So what still needs doing here? Porting my reftest harness fission changes to the marionette reftest "fork"?
Comment 9•5 years ago
|
||
I'm not sure about your changes Timothy but James is the person to ask here. Maybe you can reference the other bug where you did this work?
Comment 11•5 years ago
|
||
(In reply to Timothy Nikkel (:tnikkel) from comment #8)
So what still needs doing here? Porting my reftest harness fission changes to the marionette reftest "fork"?
Yes, I think so
Updated•5 years ago
|
Updated•5 years ago
|
Comment hidden (Intermittent Failures Robot) |
Comment 15•5 years ago
|
||
(In reply to Kris Maglione [:kmag] from comment #11)
(In reply to Timothy Nikkel (:tnikkel) from comment #8)
So what still needs doing here? Porting my reftest harness fission changes to the marionette reftest "fork"?
Yes, I think so
James, is that something you might have the time for? It's the patch from bug 1593170.
Comment 16•5 years ago
|
||
I could, but I don't think it's ideal if I'm the only person who is prepared to touch that code; I'd really like to have some familiarity with the wpt reftest implementation in the layout team, not least because they have way more expertise on the layout parts than me; I'm much more likely to miss some essential part of flushing the layout and so end up with intermittent tests and similar.
So: I think this work should be a priority, and if me doing it is the only way to get it done I can rearrange things to make that happen. But it doesn't make much sense to do it that way. Let's see what tnikkel's plans look like.
Comment 17•5 years ago
|
||
I can take this. Leaving needinfo to remind me.
Updated•5 years ago
|
Comment 18•5 years ago
|
||
Timothy, for reftests this is the place to get fixed:
https://searchfox.org/mozilla-central/rev/b712398b7fae54ef377a558d6f16dede7a7f8530/testing/marionette/listener.js#1712
I also filed bug 1625410, which is another case, but doesn't affect reftests. If you want to fix it at the same time I would appreciate. Otherwise one of us can do that. Thanks!
Comment 19•5 years ago
|
||
(In reply to Timothy Nikkel (:tnikkel) from comment #17)
I can take this. Leaving needinfo to remind me.
In that case, I'll tentatively assign this bug to you for now.
Comment 20•5 years ago
|
||
Timothy, isn't it that when we know the frame
that we could walk the browsing context tree to find the browsing context for that frame, and then use context.associatedWindow
as reference for getting windowUtils
?
Comment 21•5 years ago
|
||
associatedWindow will be null if that window is in another process. So you need to send a message to that window.
So for example here
we walk the browsing context tree and send the FlushRendering message to every child and then here
is where in the child we receive that message and do the flush.
Comment 22•5 years ago
|
||
Please note that in Marionette we still use a framescript. There is no usage of actors yet.
Comment 23•5 years ago
|
||
We will need to use actors to properly fix this bug. The reftest harness was in the same boat: it used a frame script to do most of the work. And it still does in fact, I added an actor that does that things we need to do on out of process subframes, but the actor is fairly small So we wouldn't need to change the existing framescript too much, just get it to talk to the actors. At least at first, eventually we may want to refactor it.
Comment 24•5 years ago
|
||
Bug 1635904 should also help with some of these errors.
Updated•5 years ago
|
Comment 26•4 years ago
|
||
Note that right now we don't make use of the browsing context id for identifying the various frames in content but the outer window id. With my upcoming changes on bug 1519335 we will switch to browsing contexts. So I assume this might help to get this bug fixed?
Updated•4 years ago
|
Comment hidden (Intermittent Failures Robot) |
Comment 28•4 years ago
|
||
Note that some of the work done for Tab Sharing support recently may be a useful guide here; see dom/media/systemservices/video-engine/tab-capturer.cc
Comment 29•4 years ago
|
||
(In reply to Timothy Nikkel (:tnikkel) from comment #23)
We will need to use actors to properly fix this bug. The reftest harness was in the same boat: it used a frame script to do most of the work. And it still does in fact, I added an actor that does that things we need to do on out of process subframes, but the actor is fairly small So we wouldn't need to change the existing framescript too much, just get it to talk to the actors. At least at first, eventually we may want to refactor it.
It may be possible to get away with adding a JSWindowActor just for broadcasting this flush message to every content process, if that's all we need in order to get this feature fixed and working with Fission. Would it be possible to handle that soon?
Updated•4 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Comment hidden (Intermittent Failures Robot) |
Updated•4 years ago
|
Comment 31•4 years ago
|
||
My proposal for the reftest implementation in Marionette is that we should also use the JSWindowActor similar to the original reftest harness. As such I filed bug 1648444. I wonder if we can majorly duplicate that existing actor code to make use of it in Marionette.
Timothy, what do you think?
Comment 32•4 years ago
|
||
Yeah, it shouldn't be too hard to adapt the reftest harness js window actors to marionette. Wherever something that needs to be applied to every document in the dom (like flushing layout) you instead ask the js window actor to do it. And the js window actor would have code to traverse the document tree and call that thing (ie flushing layout) on every document.
Comment 33•4 years ago
|
||
Great. So I will make use of it when working on bug 1648444 hopefully soon.
Updated•4 years ago
|
Updated•4 years ago
|
Comment hidden (Intermittent Failures Robot) |
Comment 35•4 years ago
|
||
Moving marionette-fission-mvp bugs from Fission Nightly Experiment milestone (M6b) to Fission Beta milestone (M7).
Comment 36•4 years ago
|
||
This failure didn't happen for more than a month. Lets call it WFM.
Updated•4 years ago
|
Updated•4 years ago
|
Comment 37•4 years ago
|
||
Actually I tried it with the test on bug 1695806 and this is still broken:
0:24.97 TEST_END: ERROR, expected PASS - Testing http://web-platform.test:8000/html/browsers/windows/iframe-cross-origin-print.sub.html == http://web-platform.test:8000/html/browsers/windows/iframe-nested-print-ref.html
SecurityError: Permission denied to access property "windowUtils" on cross-origin object
flushWindow@chrome://marionette/content/actors/MarionetteReftestChild.jsm:176:19
flushWindow@chrome://marionette/content/actors/MarionetteReftestChild.jsm:194:20
flushRendering@chrome://marionette/content/actors/MarionetteReftestChild.jsm:197:16
maybeResolve@chrome://marionette/content/actors/MarionetteReftestChild.jsm:123:14
paintComplete/<@chrome://marionette/content/actors/MarionetteReftestChild.jsm:147:7
paintComplete@chrome://marionette/content/actors/MarionetteReftestChild.jsm:121:12
reftestWait@chrome://marionette/content/actors/MarionetteReftestChild.jsm:98:16
Here the code that access windowUtils
:
https://searchfox.org/mozilla-central/rev/c24ecdc6f5025ea7e60d0691673de030bd5bf411/testing/marionette/actors/MarionetteReftestChild.jsm#176
Comment 38•4 years ago
|
||
Julian, could you maybe have a look at this bug?
Assignee | ||
Comment 39•4 years ago
|
||
The error makes sense considering the code at
https://searchfox.org/mozilla-central/rev/c24ecdc6f5025ea7e60d0691673de030bd5bf411/testing/marionette/actors/MarionetteReftestChild.jsm#175-197
We recursively loop on all frame elements and try to fetch windowUtils on each one of them.
So if one of those frames happens to be remote, we will throw here. Maybe there is a browsing context API we can use instead?
Comment 40•4 years ago
|
||
This is how the regular reftest code handles that
Assignee | ||
Comment 41•4 years ago
|
||
Thanks for the pointer. From what I can see, we filter out remote frames via
if (!Cu.isRemoteProxy(win.frames[i])) {
// ...
}
I read the discussion on https://phabricator.services.mozilla.com/D51343?id=184963#inline-315896 where this was initially added. I still wonder if we will incorrectly skip flushing layouts/reflows for remote frames. In any case it's better to skip them to avoid throwing, but should we have a followup bug to make sure we flush remote frames as well?
(in the meantime I'll implement a similar approach for MarionetteReftests)
Assignee | ||
Updated•4 years ago
|
Comment 42•4 years ago
|
||
(In reply to Julian Descottes [:jdescottes] from comment #41)
I read the discussion on https://phabricator.services.mozilla.com/D51343?id=184963#inline-315896 where this was initially added. I still wonder if we will incorrectly skip flushing layouts/reflows for remote frames. In any case it's better to skip them to avoid throwing, but should we have a followup bug to make sure we flush remote frames as well?
I think that would make total sense to have a follow-up if there is none yet for the original reftests. But I would like to see a reply from Timothy here.
Comment 43•4 years ago
|
||
The way it works in the regular reftest code is we send the FlushRendering message to ReftestFissionParent here
handled here
it just calls tellChildrenToFlushRendering which calls tellChildrenToFlushRenderingRecursive
that walks the browsing context tree and sends the FlushRendering message to ReftestFissionChild for every browsing context that is a process root. When ReftestFissionChild receives this message
it flushes each window it can reach without crossing a remote proxy child frame. If there is a descendant in the same process beneath a remove frame child then it will be a process root and so will get its own FlushRendering message sent to it from ReftestFissionParent.
So it's okay to skip remote child frames in ReftestFissionChild because ReftestFissionParent sends the FlushRendering message to every process root, because those remote child frames will be process roots.
So you'd have to make sure that the marionette reftest code does something similar.
Assignee | ||
Comment 44•4 years ago
|
||
Taking a similar approach to the regular Reftest actors, we recursively call flush rendering using js window actors in all remote frames.
The main difference with the regular Reftest actor is that the top frame's flushRendering is still performed as part of the reftestWait call.
The reason for that is that we perform 2 flushRendering in case there is a "reftest-wait" classname, which is easier if we keep it outside of the recursive call.
Comment 45•4 years ago
|
||
Comment 46•4 years ago
|
||
bugherder |
Updated•2 years ago
|
Comment 47•2 years ago
|
||
Description
•