1410470 - Stop using `rawObj instanceof`

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P3)

Description

[Oriol Brufau [:Oriol]]

/devtools/server/actors/object.js uses lots of `rawObj instanceof Ci.something` to detect whether `rawObj` has that `something` interface.

However, this is not great. For example, before I added a previewer for Symbol objects in bug 1391274, you could run this in a privileged console:

    Object.setPrototypeOf(Object(Symbol('a')), document.body)

Then `rawObj instanceof Ci.nsIDOMElement` was true, so the console attempted to call various Element getters, producing an error:

    TypeError: 'get nodeType' called on an object that does not implement interface Node.

I think right now this is not a problem because all non-DOM classes have a custom stringifier, but I'm not sure. And this will become problematic again if a future version of ECMAScript adds new kinds of objects.

So instead of instanceof, I think something like this could be used:

  function hasInterface(rawObj, interface) {
    try {
      QueryInterface.call(rawObj, interface);
      return true;
    } catch (err) {
      return false;
    }
  }
  hasInterface(document.body, Ci.nsIDOMElement); // true
  hasInterface(Object.create(document.body), Ci.nsIDOMElement); // false

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Oriol Brufau [:Oriol] from comment #0)
> I think right now this is not a problem because all non-DOM classes have a
> custom stringifier, but I'm not sure. And this will become problematic again
> if a future version of ECMAScript adds new kinds of objects.

Reading the description of this bug, I'm not sure I follow what the concern here is. Is the worry that there might be a crash because of objects with weird prototypes? Or is this just a QOI issue where devtools will throw when exposed to these objects? Also, what's the link between this and custom stringifiers?

Comment 2

[Oriol Brufau [:Oriol]]

(In reply to Blake Kaplan (:mrbkap) from comment #1)
> Reading the description of this bug, I'm not sure I follow what the concern
> here is. Is the worry that there might be a crash because of objects with
> weird prototypes? Or is this just a QOI issue where devtools will throw when
> exposed to these objects?

No crash, just that the object is initially identified as a DOM Element, but it is not. Not a big problem because the thrown error is catched and another previewer is used. But should not happen, the previewer should have some way to correctly identify that it's not an Element, and return false instead of throwing.

> Also, what's the link between this and custom stringifiers?

Sorry, I meant previewer, not stringifier. The link is that if there is no previewer defined for the class of the object, the console uses the previewers for the "Object" class as the default.
https://searchfox.org/mozilla-central/rev/9bab9dc5a9472e3c163ab279847d2249322c206e/devtools/server/actors/object.js#138-139

Note the DOMNode previewer that is in that default list, because DOM Nodes can have lots of different classes.

So before I added a custom previewer for the "Symbol" class, then DOMNode ran for the object, and wrongly identified it as a DOM Element because there was one in the [[Prototype]].

This problem does not happen for objects with an "Object" class (e.g. `Object.create(document.body)`) because DOMNode previewer blacklists it, it never consider objects with that class to be a DOM Node. An unreliable ad-hoc solution.
https://searchfox.org/mozilla-central/rev/9bab9dc5a9472e3c163ab279847d2249322c206e/devtools/server/actors/object.js#1786

Comment 3

[Oriol Brufau [:Oriol]]

Most cases were fixed when removing XPCOM interfaces, e.g. bug 1455676 turned

rawObj instanceof Ci.nsIDOMNode

into

Node.isInstance(rawObj)

which doesn't iterate the prototype chain. Great! isInstance was added in bug 1428531.

It seems there are just two remaining

rawObj instanceof Ci.nsIDOMWindow

which could probably be

Window.isInstance(rawObj)

But note instanceof is not the only problem when accessing rawObj, see bug 1406905.

Comment 4

[Oriol Brufau [:Oriol]]

For example, this is causing problems like this in privileged pages without xrays (like about:config)

Object.create(window, {location: {value: 1, enumerable: true}})

Expected: Object { location: 1 }
Actual: Object [object Object]

It can also run unsafe getters:

Object.create(window, {location: {get:() => {console.log('pwned!'); return {href: 'Oops'}}, enumerable: true}})

Expected: Object { location: Getter }
Actual: Object Oops and a pwned! log.

Object.create(window)

This shows Object { } as expected, but produces an error in the browser console

TypeError: 'get location' called on an object that does not implement interface Window.
    ObjectWithURL resource://devtools/server/actors/object/previewers.js:709
    _populateGripPreview resource://devtools/server/actors/object.js:238
    form resource://devtools/server/actors/object.js:179
    objectGrip resource://devtools/server/actors/webconsole.js:524
    createValueGrip resource://devtools/server/actors/object/utils.js:172
    createValueGrip resource://devtools/server/actors/webconsole.js:471
    prepareEvaluationResult resource://devtools/server/actors/webconsole.js:1282
    evaluateJS resource://devtools/server/actors/webconsole.js:1134
    makeInfallible resource://devtools/shared/ThreadSafeDevToolsUtils.js:103

Comment 5

[Oriol Brufau [:Oriol]]

Attachment: Bug 1410470 - [devtools] Stop using unreliable 'instanceof Ci.nsIDOMWindow'. r?nchevobbe,smaug

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3d8df92dce7e
[devtools] Stop using unreliable 'instanceof Ci.nsIDOMWindow'. r=nchevobbe,smaug

Comment 7

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/3d8df92dce7e