Andreas Tolfsen ❲:ato❳ Reporter
	Description • 6 years ago

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.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 1 • 6 years ago

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.

	James Graham [:jgraham]
	Comment 2 • 6 years ago

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.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 3 • 6 years ago

(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

	James Graham [:jgraham]
	Comment 4 • 6 years ago

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.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 5 • 6 years ago

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?

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 6 • 6 years ago

(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

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 7 • 6 years ago

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

	Bobby Holley (:bholley) Assignee
	Comment 8 • 6 years ago

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

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 9 • 6 years ago

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

	Bobby Holley (:bholley) Assignee
	Comment 10 • 6 years ago

MozReview-Commit-ID: XeWrs0cNL5

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 11 • 6 years ago

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

	Bobby Holley (:bholley) Assignee
	Comment 12 • 6 years ago

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

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 13 • 6 years ago

I just wanted to chime in to say that this is fantastic work, both
of you.  Thanks!

	Boris Zbarsky [:bzbarsky]
	Comment 14 • 6 years ago

Comment on attachment 9006684 [details]
Bug 1453057 - Make Xrays to Arguments objects work correctly.

Boris Zbarsky [:bzbarsky, bz on IRC] has approved the revision.

	Pulsebot
	Comment 15 • 6 years ago

Pushed by bholley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e2128a59b167
Make Xrays to Arguments objects work correctly. r=bzbarsky

	Bogdan Tara[:bogdan_tara | bogdant]
	Comment 16 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/e2128a59b167