1380506 - _findSafeGetterValues should ignore proxy objects

Status: RESOLVED FIXED

(DevTools :: Console, defect, P1)

Description

[Oriol Brufau [:Oriol]]

Open web console and enter this code:

    new Proxy({}, new Proxy({}, {get:function(_, trap){ return function(...args) { console.log(" - Trap called: ", trap, ...args); return Reflect[trap](...args); } } }))

Expand the proxy, you will see that the getPrototypeOf trap is called twice!

Either store the result in a variable in order to run the trap only once, or do not touch proxy objects at all!

Comment 1

[Oriol Brufau [:Oriol]]

OK, the problem is in https://dxr.mozilla.org/mozilla-central/rev/52285ea5e54c73d3ed824544cef2ee3f195f05e6/devtools/server/actors/object.js#317

Safe getters should definitely not be searched in proxy objects. I might fix it if I have time.

Comment 2

[Oriol Brufau [:Oriol]]

No longer blocking bug 1308566 because this problem also happened with the old console with this code:

    Object.create(new Proxy({}, new Proxy({}, {get: (target, prop) => function(...args) { console.log(prop, ...args); return Reflect[prop](...args) }})))

_findSafeGetterValues should ignore proxy objects.

Comment 3

[Oriol Brufau [:Oriol]]

Attachment: findgetter-proxy.patch

Comment 4

[Brian Grinstead [:bgrins]]

Comment on attachment 8895472 [details] [diff] [review]
findgetter-proxy.patch

Review of attachment 8895472 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for the fix.

For the test, the old debugger frontend is off by default and going to be deleted so I'd rather test the ObjectActor more directly if possible.  See tests like devtools/server/tests/unit/test_objectgrips-*.js, for instance: https://dxr.mozilla.org/mozilla-central/source/devtools/server/tests/unit/test_objectgrips-01.js. Do you think this would work? Hopefully a test like that will be easier to add since it doesn't have to touch the UI.

Comment 5

[Oriol Brufau [:Oriol]]

I don't know how this new debugger works and I don't have enough time to learn it. So here I would just add the test for the old debugger and someone else can add the new debugger tests in bug 1388831.

Comment 6

[Oriol Brufau [:Oriol]]

Comment on attachment 8895472 [details] [diff] [review]
findgetter-proxy.patch

And adding new debugger tests here would be useless because they would fail because bug 1388831 is not fixed. So requesting review again.

Comment 7

[Brian Grinstead [:bgrins]]

To be clear, I'm not suggesting to add a mochitest for the new debugger. I'm suggesting to add a unit test for the ObjectActor / ObjectClient to directly test the change. I believe something like test_objectgrips-16.js is a close example of what the test would need to do.

Comment 8

[Oriol Brufau [:Oriol]]

OK, but _findSafeGetterValues is not exported. So I should test onPrototypeAndProperties like in test_objectgrips-16.js. But the test will fail because onPrototypeAndProperties does run proxy traps before calling _findSafeGetterValues. And this bug only fixes _findSafeGetterValues. Other things should be fixed in bug 1388831.

Comment 9

[Brian Grinstead [:bgrins]]

(In reply to Oriol [:Oriol] from comment #8)
> OK, but _findSafeGetterValues is not exported. So I should test
> onPrototypeAndProperties like in test_objectgrips-16.js. But the test will
> fail because onPrototypeAndProperties does run proxy traps before calling
> _findSafeGetterValues. And this bug only fixes _findSafeGetterValues. Other
> things should be fixed in bug 1388831.

I'm not sure how a mochitest checking the variables view would be able to pass, but a unit test checking ObjectClient directly would fail?  Regardless, in that case we could add a failing test that's skipped with a comment pointing to Bug 1388831 and then once we fix up onPrototypeAndProperties in that bug it could be un-skipped: https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests#Conditional_test_functions.

Comment 10

[Oriol Brufau [:Oriol]]

I was writing the test but there is some dark magic beyond my understanding.

How come if I use

      let b = !trapDidRun;
      Assert.ok(true);

then the test works (but does not check anything), but if I use

      let b = !trapDidRun;
      Assert.ok(b);

then I get

      TEST_STATUS: Thread-5 test_proxy_grip/</< FAIL [test_proxy_grip/</< : 50] false == true
                   ./_tests/xpcshell/devtools/server/tests/unit/test_objectgrips-17.js:test_proxy_grip/</<:50
      LOG: Thread-5 INFO exiting test
      PROCESS_OUTPUT: Thread-5 (pid:13292) "JavaScript error: resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/script.js, line 356: uncaught exception: 2147500036"
      PROCESS_OUTPUT: Thread-5 (pid:13292) "DBG-TEST: resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/script.js:356: error: uncaught exception: 2147500036"
      PROCESS_OUTPUT: Thread-5 (pid:13292) "DBG-TEST: head_dbg.js observed a console message: uncaught exception: 2147500036"
      LOG: Thread-5 INFO "CONSOLE_MESSAGE: (error) [JavaScript Error: "uncaught exception: 2147500036" {file: "resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/script.js" line: 356}]"


And if I use double negation

      let b = !!trapDidRun;
      Assert.ok(b);

I get the same errors, including the FAIL false == true

Comment 11

[Oriol Brufau [:Oriol]]

This is incredible. Now I'm passing the value in an array. If I check

    do_check_eq(response.ownProperties[0].value, true);

then the server sends this packet

  "ownProperties": {"
    "0": {"
      "value": false"

If I change the check to

    do_check_eq(response.ownProperties[0].value, false);

then the server sends this packet

  "ownProperties": {"
    "0": {"
      "value": true"

And if I use

    do_check_eq(response.ownProperties[0].value, Math.random() < .5);

then the server always sends whatever boolean that will make the check fail.
This means the server know the result of the future Math.random() call!!
How can this be even possible??? The test wants to fail so much that uses knowledge from future events!!
This will be great to win the lottery, but I'm starting to question my own sanity...

Comment 12

[Oriol Brufau [:Oriol]]

Ah, I get it. Sadly I won't win the lottery. What I didn't notice is that the test code seems to run multiple times. And probably the grips are cached or something so they don't always run proxy traps. I was always looking at the last test, which was the failed one, of course.

The problem is that I thought that between this bug and bug 1378207 it would be enough to handle proxies in the old console, and the new one would only need bug 1388831.

But in the test it seems there are no xrays, so various instanceof in object.js run proxy traps, e.g in
https://dxr.mozilla.org/mozilla-central/rev/4c5fbf49376351679dcc49f4cff26c3c2e055ccc/devtools/server/actors/object.js#1588

Then, either

 - There should always be xrays in there, and the lack of them is a bug of the unit test.
 - xrays should not be expected. Then the changes that I originally planned to do in bug 1298697 are still necessary.
   In this case I will dupe this bug to bug 1298697 and I will do all the work in there.

Which is it?

Comment 13

[Oriol Brufau [:Oriol]]

Ah, no, sorry, the problem is not the lack of xrays, the problem is that chrome objects are considered safe for some reason:
https://dxr.mozilla.org/mozilla-central/rev/4c5fbf49376351679dcc49f4cff26c3c2e055ccc/devtools/shared/DevToolsUtils.js#251-255

So, the question is: can I remove this? Otherwise, how can I create a sandbox in the unit test so that the proxy is no longer considered safe?

Comment 14

[Oriol Brufau [:Oriol]]

Attachment: findgetter-proxy.patch

Now with the unit test. Some notes:

 - I don't use getPrototypeAndProperties directly on the proxy object, this would run proxy traps.
 - I run the code in a null principal sandbox. Objects from a system principal are considered safe even if they are proxy objects, so object.js runs traps.

Comment 15

[Oriol Brufau [:Oriol]]

Attachment: findgetter-proxy.patch

I forgot to remove some useless imports.

Comment 16

[Brian Grinstead [:bgrins]]

Comment on attachment 8896298 [details] [diff] [review]
findgetter-proxy.patch

Review of attachment 8896298 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you, this looks great! Can you revert the changes to browser_dbg_variables-view-07.js and doc_proxy.html? I'm fine with just the server side test as we can handle frontend tests for OI in Bug 1388831.

::: devtools/server/actors/object.js
@@ +319,5 @@
>      let obj = this.obj;
>      let level = 0, i = 0;
>  
> +    // Do not search safe getters in proxy objects.
> +    if (obj.isProxy) {

This condition doesn't appear to be needed. The `while` condition below handles the case where obj is a proxy

Comment 17

[Oriol Brufau [:Oriol]]

Comment on attachment 8896298 [details] [diff] [review]
findgetter-proxy.patch

Review of attachment 8896298 [details] [diff] [review]:
-----------------------------------------------------------------

I would not revert the old debugger tests because in the old console `_findSafeGetterValues` is called via `GenericObject` instead of `onPrototypeAndProperties`. And `GenericObject` is not exported so I can't test it directly with unit test.

::: devtools/server/actors/object.js
@@ +319,5 @@
>      let obj = this.obj;
>      let level = 0, i = 0;
>  
> +    // Do not search safe getters in proxy objects.
> +    if (obj.isProxy) {

I added this condition because the code just below uses `obj = obj.proto`, and `obj` can be a proxy. But one could argue that `obj` should not be a proxy here. The old console did not run this function directly on proxies, and the new console will probably stop doing so in bug 1388831.

Comment 18

[Brian Grinstead [:bgrins]]

(In reply to Oriol [:Oriol] from comment #17)
> Comment on attachment 8896298 [details] [diff] [review]
> findgetter-proxy.patch
> 
> Review of attachment 8896298 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I would not revert the old debugger tests because in the old console
> `_findSafeGetterValues` is called via `GenericObject` instead of
> `onPrototypeAndProperties`. And `GenericObject` is not exported so I can't
> test it directly with unit test.

Fair enough, but the plan is to ship the new console frontend in 57. That means this would be providing test coverage only in nightly builds for the rest of this cycle, and there's almost no activity on either the variables view or the old frontend. So I'd prefer to not review those test changes if possible.

> ::: devtools/server/actors/object.js
> @@ +319,5 @@
> >      let obj = this.obj;
> >      let level = 0, i = 0;
> >  
> > +    // Do not search safe getters in proxy objects.
> > +    if (obj.isProxy) {
> 
> I added this condition because the code just below uses `obj = obj.proto`,
> and `obj` can be a proxy. But one could argue that `obj` should not be a
> proxy here. The old console did not run this function directly on proxies,
> and the new console will probably stop doing so in bug 1388831.

If obj is a proxy then it's class will be "Proxy" (see line ~90), so it won't hit that case. And if that code changes and we do hit it then this test should catch it :).

Comment 19

[Oriol Brufau [:Oriol]]

(In reply to Brian Grinstead [:bgrins] from comment #18)
> Fair enough, but the plan is to ship the new console frontend in 57. That
> means this would be providing test coverage only in nightly builds for the
> rest of this cycle, and there's almost no activity on either the variables
> view or the old frontend. So I'd prefer to not review those test changes if
> possible.

OK.

> If obj is a proxy then it's class will be "Proxy" (see line ~90), so it
> won't hit that case. And if that code changes and we do hit it then this
> test should catch it :).

Nope. `g.class = "Proxy"` just lies and sets a Proxy class for the grip. But the debugger object of the proxy still has the same class as its target.

Comment 20

[Oriol Brufau [:Oriol]]

In fact it might make sense for the debugger object to directly say that the class is Proxy. I looked a bit into it but I didn't know what exactly to change, so in bug 1274657 I ended up using this `g.class = "Proxy"` hack.

Comment 21

[Brian Grinstead [:bgrins]]

(In reply to Oriol Brufau [:Oriol] from comment #19)
> > If obj is a proxy then it's class will be "Proxy" (see line ~90), so it
> > won't hit that case. And if that code changes and we do hit it then this
> > test should catch it :).
> 
> Nope. `g.class = "Proxy"` just lies and sets a Proxy class for the grip. But
> the debugger object of the proxy still has the same class as its target.

Ah, right. Makes sense to leave it in. Maybe the test case should cover this condition?

Comment 22

[Oriol Brufau [:Oriol]]

Attachment: findgetter-proxy-v2.patch

Removing old debugger tests.

(In reply to Brian Grinstead [:bgrins] from comment #21)
> Ah, right. Makes sense to leave it in. Maybe the test case should cover this
> condition?

The problem is that I think I can only test _findSafeGetterValues via onPrototypeAndProperties. But onPrototypeAndProperties calls ownKeys and getPrototypeOf traps. So the test would fail.

Comment 23

[Brian Grinstead [:bgrins]]

Comment on attachment 8897253 [details] [diff] [review]
findgetter-proxy-v2.patch

Review of attachment 8897253 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this looks good to me. Please update the review message to include the bug id and r=bgrins.

Here's a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=89d8e1381069ad563261deda23696369890272c5. Had to skip Windows on that push since I was getting infrastructure errors on try with it included.

Comment 24

[Oriol Brufau [:Oriol]]

Attachment: findgetter-proxy-v2.patch

(In reply to Brian Grinstead [:bgrins] from comment #23)
> Please update the review message to include the bug id and r=bgrins.

I never did this before, I thought this was up to whoever landed the patch.
Do I need to request review again?

Comment 25

[Brian Grinstead [:bgrins]]

Comment on attachment 8897449 [details] [diff] [review]
findgetter-proxy-v2.patch

Review of attachment 8897449 [details] [diff] [review]:
-----------------------------------------------------------------

There's generally no need to re-request review if someone gives you an r+ and asks for a minor change. As for setting the bug # and reviewer in the commit message, it makes it easier for whoever is checking it in: https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F.

Comment 26

[Oriol Brufau [:Oriol]]

The try only has some failures which seem unrelated intermittent things.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7dcd9537f6fa
Prevent the console from searching safe getters in proxy objects. r=bgrins

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7dcd9537f6fa