1574508 - 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)

Status: RESOLVED FIXED

(Testing :: Marionette Client and Harness, defect, P3)

Description

[Treeherder Bug Filer]

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

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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:

https://searchfox.org/mozilla-central/rev/3a61fb322f74a0396878468e50e4f4e97e369825/layout/tools/reftest/reftest-content.js#534-539

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?

Comment 2

[Intermittent Failures Robot]

23 failures in 4839 pushes (0.005 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• try: 23

Platform breakdown:

• linux64: 1
• windows7-32: 9
• windows10-64: 9
• windows10-aarch64: 4

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1574508&startday=2019-08-12&endday=2019-08-18&tree=all

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 the getBoundingClientRect 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

[Matt Woodrow (:mattwoodrow)]

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

[Kris Maglione [:kmag]]

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

[Matt Woodrow (:mattwoodrow)]

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

[Chris Peterson [:cpeterson]]

Tracking for Fission milestone M6 because we want to be able to run Marionette tests with Fission before enabling in Nightly.

Comment 8

[Timothy Nikkel (:tnikkel)]

So what still needs doing here? Porting my reftest harness fission changes to the marionette reftest "fork"?

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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 10

[Timothy Nikkel (:tnikkel)]

Bug 1593170.

Comment 11

[Kris Maglione [:kmag]]

(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

Comment 13

[Intermittent Failures Robot]

1 failures in 5336 pushes (0.0 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• mozilla-central: 1

Platform breakdown:

• linux1804-64-qr: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1574508&startday=2020-03-09&endday=2020-03-15&tree=all

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(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

[James Graham [:jgraham]]

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

[Timothy Nikkel (:tnikkel)]

I can take this. Leaving needinfo to remind me.

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

[Chris Peterson [:cpeterson]]

(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

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

[Timothy Nikkel (:tnikkel)]

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

https://searchfox.org/mozilla-central/rev/dc4560dcaafd79375b9411fdbbaaebb0a59a93ac/layout/tools/reftest/ReftestFissionParent.jsm#11

we walk the browsing context tree and send the FlushRendering message to every child and then here

https://searchfox.org/mozilla-central/rev/dc4560dcaafd79375b9411fdbbaaebb0a59a93ac/layout/tools/reftest/ReftestFissionChild.jsm#96

is where in the child we receive that message and do the flush.

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Please note that in Marionette we still use a framescript. There is no usage of actors yet.

Comment 23

[Timothy Nikkel (:tnikkel)]

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

[Matt Woodrow (:mattwoodrow)]

Bug 1635904 should also help with some of these errors.

Comment 25

[Chris Peterson [:cpeterson]]

Tracking WPT Fission bugs for Fission M6b (Q2)

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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?

Comment 27

[Intermittent Failures Robot]

1 failures in 5211 pushes (0.0 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• autoland: 1

Platform breakdown:

• windows7-32: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1574508&startday=2020-06-08&endday=2020-06-14&tree=all

Comment 28

[Randell Jesup [:jesup] (needinfo me)]

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

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

(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?

Comment 30

[Intermittent Failures Robot]

1 failures in 3410 pushes (0.0 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• mozilla-central: 1

Platform breakdown:

• windows10-64-shippable: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1574508&startday=2020-09-07&endday=2020-09-13&tree=all

Comment 31

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

[Timothy Nikkel (:tnikkel)]

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

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Great. So I will make use of it when working on bug 1648444 hopefully soon.

Comment 34

[Intermittent Failures Robot]

1 failures in 3760 pushes (0.0 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• autoland: 1

Platform breakdown:

• windows10-64: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1574508&startday=2020-09-14&endday=2020-09-20&tree=all

Comment 35

[Chris Peterson [:cpeterson]]

Moving marionette-fission-mvp bugs from Fission Nightly Experiment milestone (M6b) to Fission Beta milestone (M7).

Comment 36

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This failure didn't happen for more than a month. Lets call it WFM.

Comment 37

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Julian, could you maybe have a look at this bug?

Comment 39

[Julian Descottes [:jdescottes]]

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

[Timothy Nikkel (:tnikkel)]

This is how the regular reftest code handles that

https://searchfox.org/mozilla-central/rev/c24ecdc6f5025ea7e60d0691673de030bd5bf411/layout/tools/reftest/ReftestFissionChild.jsm#231

Comment 41

[Julian Descottes [:jdescottes]]

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)

Comment 42

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(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

[Timothy Nikkel (:tnikkel)]

The way it works in the regular reftest code is we send the FlushRendering message to ReftestFissionParent here

https://searchfox.org/mozilla-central/rev/63fcc3f1a2cc73488d8986f4cf91fce2cd4b7564/layout/tools/reftest/reftest-content.js#468

handled here

https://searchfox.org/mozilla-central/rev/63fcc3f1a2cc73488d8986f4cf91fce2cd4b7564/layout/tools/reftest/ReftestFissionParent.jsm#157

it just calls tellChildrenToFlushRendering which calls tellChildrenToFlushRenderingRecursive

https://searchfox.org/mozilla-central/rev/63fcc3f1a2cc73488d8986f4cf91fce2cd4b7564/layout/tools/reftest/ReftestFissionParent.jsm#11

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

https://searchfox.org/mozilla-central/rev/63fcc3f1a2cc73488d8986f4cf91fce2cd4b7564/layout/tools/reftest/ReftestFissionChild.jsm#197

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.

Comment 44

[Julian Descottes [:jdescottes]]

Attachment: Bug 1574508 - [marionette] reftest: call flushRendering in remote frames using actors

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/73cbf32ab40a
[marionette] reftest: call flushRendering in remote frames using actors r=marionette-reviewers,jgraham

Comment 46

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/73cbf32ab40a

Comment 47

[BMO Automation]

Moving bug to Testing::Marionette Client and Harness component per bug 1815831.