Closed Bug 1457711 Opened 2 years ago Closed 2 years ago

Property previewer can throw

Categories

(DevTools :: Console, defect)

defect
Not set

Tracking

(firefox61 fixed)

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: Oriol, Assigned: Oriol)

Details

Attachments

(1 file)

Create a revoked proxy in the console

    var p = Proxy.revocable({}, {});
    p.revoke();

Then write this:

    p.proxy.

The previewer attempts to list properties, but this produces an error in the browser console:

Handler function JSPropertyProvider threw an exception: TypeError: illegal operation attempted on a revoked proxy
Stack: getProperties@resource://devtools/shared/base-loader.js -> resource://devtools/shared/webconsole/js-property-provider.js:485:12
getMatchedPropsImpl@resource://devtools/shared/base-loader.js -> resource://devtools/shared/webconsole/js-property-provider.js:395:17
getMatchedPropsInDbgObject@resource://devtools/shared/base-loader.js -> resource://devtools/shared/webconsole/js-property-provider.js:364:10
JSPropertyProvider@resource://devtools/shared/base-loader.js -> resource://devtools/shared/webconsole/js-property-provider.js:272:10
exports.makeInfallible/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:109:14
onAutocomplete@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/webconsole.js:1088:18
onPacket@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:1778:15
send/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/transport.js:569:13
exports.makeInfallible/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:109:14
exports.makeInfallible/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:109:14
Line: 485, column: 12


This also happens for some rare objects like Cu.Sandbox
I'm not a fan of allowing the console to run proxy traps, but I guess that if the user is already attempting to get some property, it may be acceptable. But I also added a test for Cu.Sandbox in case proxy objects are eventually blacklisted.
Comment on attachment 8971822 [details]
Bug 1457711 - Catch errors thrown by console's property previewer;

https://reviewboard.mozilla.org/r/240568/#review247062

Sorry for the long delay Oriol.
The patch looks good to me, I only have a few nits and 2 requests in the test to make it more readable.
Thanks a lot !

::: commit-message-d2d51:1
(Diff revision 1)
> +Bug 1457711 - Catch errors thrown by console's property previewer

please add "; r=nchevobbe." at the end of the sentence

::: devtools/shared/webconsole/test/test_jsterm_autocomplete.html:26
(Diff revision 1)
>    return new Promise((resolve, reject) => {
>      gState.client.evaluateJSAsync(input, resolve, options);
>    });
>  }
>  
>  function autocompletePromise(str, cursor, frameActor) {

nit: since we're modifying this test, what would you think of making the default `cursor` argument `str.length + 1` ? Most of the time calls to this function have the autocompletion triggered at the end of the string.

This would make the test a lot easier to read

::: devtools/shared/webconsole/test/test_jsterm_autocomplete.html:165
(Diff revision 1)
>    is(response.matches.length, MAX_AUTOCOMPLETIONS, "matches.length is MAX_AUTOCOMPLETIONS");
>  
>    nextTest();
>  }
>  
> +async function doAutocompleteProxy1() {

nit: could we find better name than Proxy1/2 ?

::: devtools/shared/webconsole/test/test_jsterm_autocomplete.html:190
(Diff revision 1)
> +}
> +
> +async function doAutocompleteSandbox() {
> +  // Check that completion provides inherited properties even if [[OwnPropertyKeys]] throws.
> +  // `Cu` is not defined in workers, then we can't test `Cu.Sandbox`
> +  if ((await evaluateJS("Cu")).result.type == "object") {

Could the "isWorker" test be more explicit ? Here, I could imagine the test turning false even in a tab, which would make us lose the test coverage.

we could bind the call to `onAttach` in line 37 so it calls it with an extra parameter `isWorker` that we could pass to each test.
Attachment #8971822 - Flags: review?(nchevobbe) → review+
Keywords: checkin-needed
Pushed by shindli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/65efb76dcc6c
Catch errors thrown by console's property previewer; r=nchevobbe
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/65efb76dcc6c
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.