Closed Bug 1453057 Opened 2 years ago Closed 2 years ago

Investigate missing Symbol.iterator on Arguments in sandbox

Categories

(Core :: XPConnect, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: ato, Assigned: bholley)

References

Details

Attachments

(1 file)

As I was working on https://bugzilla.mozilla.org/show_bug.cgi?id=1453009
I discovered that the iterator protocol for the Arguments object
disappears when it is returned from a script run in a sandbox.

Due to the level of complexity in this part of Marionette it needs
to be further investigated, but I’m filing this bug as a follow-up
on the test annotated with EXPECTED-FAIL from that bug.
Depends on: 1453009
Priority: -- → P3
Priority: P3 → P2
The test was marked as expected fail by a wptsync job. Here the change:
https://hg.mozilla.org/mozilla-central/diff/61159a96c9a5/testing/web-platform/meta/webdriver/tests/execute_async_script/collections.py.ini

Sadly the bug number has been removed so it makes it very very hard to find this bug. James, can we please make sure to keep bug numbers if present? Thanks.
Flags: needinfo?(james)
The bug number wasn't removed. The bug that introducecd the change from upstream is in an earlier commit. For whatever reason we didn't manage to get the metadata change from the try run for that specific commit, so we only got the tests being disabled in the final landing commit. It's not terribly easy to automatically reverse engineer which commit introduced the change at that stage, but you can surely work it out from the commit logs.
Flags: needinfo?(james)
(In reply to James Graham [:jgraham] from comment #2)
Sorry, but I don't understand your comment. The new test module was introduced by Andreas on bug 1453009, and he added the bug comment next to the expected failure, but with your push [1] for the wptsync the bug comment has been removed.

Maybe I misunderstand what you are trying to say regarding try. But as I can see you did the push.

[1] https://hg.mozilla.org/integration/mozilla-inbound/rev/61159a96c9a5
Oh, I see.

Comments aren't preserved; if you want to add a bug annotation then the convention is

bug: <number>

i.e. a real field, which can also be machine read in the future.
Good to know. Thanks! So now we only have to get it fixed. Andreas, will you have time for it or should take a look next week?
Depends on: 1461969
(In reply to Henrik Skupin (:whimboo) from comment #5)
> Good to know. Thanks! So now we only have to get it fixed. Andreas, will you
> have time for it or should take a look next week?

https://bugzilla.mozilla.org/show_bug.cgi?id=1461969
(In reply to Andreas Tolfsen ❲:ato❳ from comment #0)
> As I was working on https://bugzilla.mozilla.org/show_bug.cgi?id=1453009
> I discovered that the iterator protocol for the Arguments object
> disappears when it is returned from a script run in a sandbox.

As it looks like it doesn't disappear but it's somewhat not allowed. When I run this test with a debug build of Firefox I can see the following line in the log output:

> [Child 62227, Main Thread] WARNING: Silently denied access to property Symbol.iterator: value is callable (@chrome://marionette/content/evaluate.js:253:12): file /builds/worker/workspace/build/src/js/xpconnect/wrappers/XrayWrapper.cpp, line 216

Which is logged inside of `ReportWrapperDenial()` and is called most likely by:

https://dxr.mozilla.org/mozilla-central/rev/c2e3be6a1dd352b969a45f0b85e87674e24ad284/js/xpconnect/wrappers/XrayWrapper.cpp#345

The Arguments object where this happens looks like:

> [object Arguments] {"0":"foo","1":"bar"}

So I'm not sure what `value is callable` means here.

Bobby, you implemented those checks already a while ago. Maybe you could give us a hint? Thanks.
Flags: needinfo?(bobbyholley)
(In reply to Henrik Skupin (:whimboo) from comment #7)
> > [Child 62227, Main Thread] WARNING: Silently denied access to property Symbol.iterator: value is callable (@chrome://marionette/content/evaluate.js:253:12): file /builds/worker/workspace/build/src/js/xpconnect/wrappers/XrayWrapper.cpp, line 216

This error means that you're inspecting a non-DOM object via an XrayWrapper, and the XrayWrapper is denying access. Non-DOM XrayWrappers are a bit funky (because there's no clear distinction between "trusted platform state" and "untrusted JS state"), but the general principle of XrayWrappers is to avoid unintentionally executing untrusted code, so we avoid exposing any properties that are callable (hence the error message).

The Right Fix depends a lot on the context of what's happening, which isn't obvious from this bug. Waiving Xrays is certainly an option though.
Flags: needinfo?(bobbyholley)
The test which fails here is:

https://dxr.mozilla.org/mozilla-central/rev/c2e3be6a1dd352b969a45f0b85e87674e24ad284/testing/web-platform/tests/webdriver/tests/execute_async_script/collections.py#17-25

It's a script which gets evaluated in a sandbox, and should return us the arguments of the function call.

https://dxr.mozilla.org/mozilla-release/rev/e9f06fd63988182f9f79f223a295b57acf81cee8/testing/marionette/evaluate.js#137

While the evaluation is working fine, it fails later because there is no Symbol.iterator present:

https://dxr.mozilla.org/mozilla-release/rev/e9f06fd63988182f9f79f223a295b57acf81cee8/testing/marionette/evaluate.js#273

We make use of `wantXrays` in that same file when creating the sandbox, but whatever value is set I can still see the warning.

I also don't see what is actually callable for the Arguments instance.

Maybe the above details gives you more information to understand the problem better.

Thanks
Flags: needinfo?(bobbyholley)
MozReview-Commit-ID: XeWrs0cNL5
Assignee: nobody → bobbyholley
Flags: needinfo?(bobbyholley)
Thanks Bobby! Please note that when you want to push this change, it should change the expected test result for the following test:

https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/webdriver/tests/execute_async_script/collections.py

To not cause a backout due to a perma failure you will also have to remove the following manifest file:

https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/meta/webdriver/tests/execute_async_script/collections.py.ini
Component: Marionette → XPConnect
Product: Testing → Core
(In reply to Henrik Skupin (:whimboo) from comment #11)
> Thanks Bobby! Please note that when you want to push this change, it should
> change the expected test result for the following test:
> 
> https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/
> webdriver/tests/execute_async_script/collections.py
> 
> To not cause a backout due to a perma failure you will also have to remove
> the following manifest file:
> 
> https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/meta/
> webdriver/tests/execute_async_script/collections.py.ini

Added that to the patch - thanks!
I just wanted to chime in to say that this is fantastic work, both
of you.  Thanks!
Comment on attachment 9006684 [details]
Bug 1453057 - Make Xrays to Arguments objects work correctly.

Boris Zbarsky [:bzbarsky, bz on IRC] has approved the revision.
Attachment #9006684 - Flags: review+
Pushed by bholley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e2128a59b167
Make Xrays to Arguments objects work correctly. r=bzbarsky
https://hg.mozilla.org/mozilla-central/rev/e2128a59b167
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.