Closed Bug 1477765 Opened 6 years ago Closed 6 years ago

Error when expanding the XULDocumentPrototype in the Browser Console `'get documentReadyForIdle' called on an object that does not implement interface Document`

Categories

(DevTools :: Object Inspector, defect)

Product:
DevTools
Component:
Object Inspector
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox63 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

Attachments

(1 file)

Bug 1477765 - Handle rejected Promises when inspecting an object;
59 bytes, text/x-review-board-request
jimb
: review+
Details 
STR
- Open Browser Console
- Evaluate `document`
- Expand the object and scroll all the way to the bottom
- Expand `<prototype>: XULDocumentPrototype`

I see a warning:
```
Ignoring get or set of property that has [LenientThis] because the “this” object is incorrect. object.js:381:23
```

And an error:

```
TypeError: 'get documentReadyForIdle' called on an object that does not implement interface Document.[Learn More]
object.js:381:24
```

From https://bugzilla.mozilla.org/show_bug.cgi?id=1475342#c5:

I'm not sure what the fix is. I guess we could try/catch getter evaluation, although it's not clear to me if we should be evaluating getters on the prototype object in the first place (and if so, if they should be evaluated with the actual object or the prototype object).
There's some discussion about this going on at https://bugzilla.mozilla.org/show_bug.cgi?id=1475342#c6 as well - going to move that here since a fix should ultimately land in this bug:

```
So... The code in _findSafeGetterValues expects this bit:

        let result = getter.call(this.obj);

to return a completion record (so a thing that can have "throw" as a property), not an actual value, and then skips over completion records that threw.  But the error in that try push seems to suggest we're actually getting an exception here somehow????
```
Flags: needinfo?(jimb)
(In reply to Brian Grinstead [:bgrins] from comment #2)
> There's some discussion about this going on at
> https://bugzilla.mozilla.org/show_bug.cgi?id=1475342#c6 as well - going to
> move that here since a fix should ultimately land in this bug:
> 
> ```
> So... The code in _findSafeGetterValues expects this bit:
> 
>         let result = getter.call(this.obj);
> 
> to return a completion record (so a thing that can have "throw" as a
> property), not an actual value, and then skips over completion records that
> threw.  But the error in that try push seems to suggest we're actually
> getting an exception here somehow????
> ```

The try push in question is https://treeherder.mozilla.org/#/jobs?repo=try&revision=fadcd470c9ecd167d58d91c691b14ec96c988cfa&selectedJob=189365223, and there's also easy STR in Comment 0 here.

I also thought we should be able to call the debugger API calls without them ever throwing and that exceptions would go out into the 'throw' property. I see one exception from this page https://developer.mozilla.org/en-US/docs/Tools/Debugger-API/Debugger.Object ("If the referent is not callable, throw a TypeError"). It's not clear to me if that's the case we are hitting:

```
call(this,argument, …)
    If the referent is callable, call it with the giventhis value andargument values, and return a completion value describing how the call completed.This should be a debuggee value, or { asConstructor: true } to invoke the referent as a constructor, in which case SpiderMonkey provides an appropriate this value itself. Eachargument must be a debuggee value. All extant handler methods, breakpoints, and so on remain active during the call. If the referent is not callable, throw a TypeError. This function follows the invocation function conventions.
```
You won't catch this with the debugger API, because the getter doesn't throw!

  XULDocument.prototype.documentReadyForIdle instanceof Promise; // true

More or less, it's like if the getter was something like

  ({
    get documentReadyForIdle() {
      console.error("Foo");
      return 123;
    }
  })

The real solution? Stop assuming that native getters don't have side-effects, and implement some way to really check whether running some code has side-effects. See bug 1460518.
(In reply to Brian Grinstead [:bgrins] from comment #3)
> I also thought we should be able to call the debugger API calls without them
> ever throwing and that exceptions would go out into the 'throw' property.

Yes, that's definitely the intent: exceptions thrown by debuggee content should not propagate into debugger code across "invocation functions" like Debugger.Object.prototype.call, which I assume is what you're using here. If there's an error this is getting propagated out, we'd like to know about it.
(In reply to Oriol Brufau [:Oriol] from comment #4)
> The real solution? Stop assuming that native getters don't have
> side-effects, and implement some way to really check whether running some
> code has side-effects. See bug 1460518.

If there were a good way to do this, it would definitely be the preferred approach.
Flags: needinfo?(jimb)
Oh, this is a promise-returning getter?  In that case, this is basically bug 1438015.

> Stop assuming that native getters don't have side-effects

Generally speaking, for the web platform, this is a correct assumption.  The issue here is not side-effects.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #7)
> Oh, this is a promise-returning getter?  In that case, this is basically bug
> 1438015.

OK, reading that bug it looks like you bumped into this exact getter in Bug 1437921 / https://searchfox.org/mozilla-central/rev/d160ac574a5798010eda4e33e91ee628aa268a33/devtools/shared/webconsole/test/test_object_actor_native_getters_lenient_this.html#73.

From https://bugzilla.mozilla.org/show_bug.cgi?id=1438015#c3:
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #3)
> Jim and I had a long conversation about this on IRC, fwiw; I was assuming he
> would follow up here.
> 
> He recommended changing _findSafeGetterValues to treat an already-rejected
> return value promise just like a thrown exception is treated.

It's not clear to me exactly how to do that. How can we check if the promise is already rejected without calling the getter first inside _findSafeGetterValues?
(In reply to Brian Grinstead [:bgrins] from comment #8)
> > He recommended changing _findSafeGetterValues to treat an already-rejected
> > return value promise just like a thrown exception is treated.
> 
> It's not clear to me exactly how to do that. How can we check if the promise
> is already rejected without calling the getter first inside
> _findSafeGetterValues?

I was confusing _findSafeGetters and _findSafeGetterValues - in the latter we *do* call the getter. Still, I'm not sure practically how to inspect the rejected state in _findSafeGetterValues. I assume we are talking about this line and the subsequent "throw" check: https://searchfox.org/mozilla-central/rev/ad36eff63e208b37bc9441b91b7cea7291d82890/devtools/server/actors/object.js#381.
OK so it sounds like this is due to an unhandled promise rejection. What's not clear to me in:

> He recommended changing _findSafeGetterValues to treat an already-rejected
> return value promise just like a thrown exception is treated.

Is that once we call `getter.call(this.obj);` (which AFAICT we need to do in order to determine if the Promise is rejected), we've already created the unhandled rejection. So IIUC the suggestion here is basically:

1) Read the `return` value to see if it's a rejected promise (something like `getterValue.class == "Promise" && getterValue.promiseState == "rejected")`)
2) Somehow silence the rejection. What does that practically look like? Call catch() on the object through Debugger API? Access unsafeDereference and call catch()? Something else?
Flags: needinfo?(jimb)
Attachment #8994361 - Flags: feedback?(oriol-bugzilla)
Comment on attachment 8994361 [details]
Bug 1477765 - Handle rejected Promises when inspecting an object;

https://reviewboard.mozilla.org/r/258938/#review265920

::: devtools/server/actors/object.js:396
(Diff revision 1)
> -          }
> +        }
> +
> +        if (getterValue && (getterValue.class == "Promise" &&
> +                            getterValue.promiseState == "rejected")) {
> +          // XXX: How should we properly silence the Promise rejection?
> +          getterValue.unsafeDereference().catch(e=>e);

Ah, just an unhandled promise rejection, I thought this was more like a [LenientThis].

Well, I don't like accessing `catch` on the unsafe raw value. `Promise.prototype.catch.call` can be used instead.

However, that's still observable:

```js
var p = Promise.reject();
Object.defineProperty(p, "constructor", {get() { console.error("observable") }});

Promise.prototype.then.call(p, undefined, () => {}); // observable
Promise.prototype.catch.call(p, () => {}); // observable
```

But since the getter is considered safe, I guess the promise returned by it can be trusted to not do such things.

So I guess the patch is acceptable. But probably it would be better to add new Debugger.Object methods to do this kind of promise operations.
> However, that's still observable:

Yes, but is our "unsafe raw value" actually unsafe or an Xray?  Doing .catch() on an Xray would not be page-observable if we're doing it right.

If we have a Xray waiver instead, then the .catch() would indeed still be observable...  In that case we would indeed need new Debugger APIs for catching promises.
Okay, just to clarify:

The call (via the Debugger API) of the accessor property's `get` function here:
https://searchfox.org/mozilla-central/rev/d160ac574a5798010eda4e33e91ee628aa268a33/devtools/server/actors/object.js#381

produces a completion value { return: P } where P is a rejected promise.

What Oriol said in comment 4, however, is that the call also logs an error to the console, as a side-effect.

My first question is, why does it log that error? It looks like it thinks the `this` value isn't right? Why would that use of Debugger.Object.prototype.call not give it the right sort of `this` object? It's calling an accessor it fetched right off that object's prototype chain. The second error message also suggests the reciever isn't right.

My second question is, if it returns a rejected promise, why doesn't the property's value simply display the way any other rejected promise would? "Promise { <state>: "rejected" }" or what have you?

Third question: I thought that creating a rejected promise was okay, but if we let a rejected promise get GC'd, then that caused the error to be logged. But my experiments in the console suggest that simply creating any rejected promise at all logs an error immediately. Is this expected behavior?
Flags: needinfo?(jimb) → needinfo?(bgrinstead)
Oh, I see what's going on: we're inspecting a prototype, not a real object. That is indeed not something that the getter can be properly applied to.
> Is this expected behavior?

Yes, we moved away from the GC-dependent behavior.  Now I believe if a promise is rejected we wait until run-to-completion and microtasks end and if it still has no handlers we log the error before returning to the event loop.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #13)
> Yes, but is our "unsafe raw value" actually unsafe or an Xray?  Doing
> .catch() on an Xray would not be page-observable if we're doing it right.

documentReadyForIdle is ChromeOnly, so no Xrays.
https://searchfox.org/mozilla-central/rev/d160ac574a5798010eda4e33e91ee628aa268a33/dom/webidl/Document.webidl#409-410
> documentReadyForIdle is ChromeOnly, so no Xrays.

You mean no waivers, right?  ChromeOnly things are _definitely_ exposed over Xrays from chrome code.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #18)
> > documentReadyForIdle is ChromeOnly, so no Xrays.
> 
> You mean no waivers, right?  ChromeOnly things are _definitely_ exposed over
> Xrays from chrome code.

I'm not an expert in this, but my understanding was that Xrays appear when chrome code accesses an object from a less privileged principal. So if they are both chrome, then there are no Xrays?

  var s = Cu.Sandbox(Cu.getObjectPrincipal({}));
  Cu.evalInSandbox("var obj = {get foo(){return 123}}", s);
  s.obj.foo; // 123

  var s = Cu.Sandbox(null);
  Cu.evalInSandbox("var obj = {get foo(){return 123}}", s);
  s.obj.foo; // undefined, XrayWrapper denied access to property "foo"

And I remember some issues with the console where it assumes that objects are protected by Xrays but this assumption is false in privileged pages like about:config, and then it's easy to make the console fail to properly inspect objects by defining getters or modifying prototype chains.
> So if they are both chrome, then there are no Xrays?

That is true, but that has nothing to do with chromeonly APIs.

In this case, the rejected promise may be a chrome object or not, depending on whether the this.obj is an Xray or not.

You're right that if we're inspecting a chrome page then we are not protected by Xrays and just have to trust those pages to not mess with things.
I think all three questions from Comment 14 have been answered, so clearing the needinfo. Is the thing I'm doing in the patch: `getterValue.unsafeDereference().catch(e=>e);` an OK way to make sure the rejection is handled, or is there a better/safer way to handle this through the Debugger API?
Flags: needinfo?(bgrinstead) → needinfo?(jimb)
See Also: → 1478076
Yes, all my questions are answered.

I don't know of any way to mark a promise as caught via the Debugger API. I think it needs one. unsafeDereference().catch(e=>e) seems like it could be tricked by content overriding the 'catch' method. Filed as bug 1478076.
Flags: needinfo?(jimb)
Jim, I made a change so that it only attempts to catch() when `DevToolsUtils.isSafeJSObject` is true (with a note to change this to a debugger API call once Bug 1478076 lands). Does that work for you?
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
I'm also assuming we can dupe bug 1438015 to this one if we go with the patch here.
You know, the more I think about this, the less I like the entire approach.

Suppose the developer tools try to apply an accessor and get a rejected promise. How can the tools tell whether they should mark it as caught or not?

If the rejected promise was created specifically as a result of trying to apply the accessor, as in the case in comment 0, then of course one should mark it as handled; the promise would never have existed if we hadn't tried to inspect the object.

But what if the accessor happens to be one that returns some user-provided value, and that value happens to be a promise that was recently rejected by the user's code? The developer tools must not mark such a promise as caught, because that would suppress the warning we've worked hard not to bury.

I think that the accessor should throw an exception, rather than returning a rejected promise. Given the nature of the exception being reported here, it seems better that the throw should happen as soon as the accessor is applied, not later when the promise is handled.

Boris, what do you think of changing the API in this way?
Flags: needinfo?(bzbarsky)
Comment on attachment 8994361 [details]
Bug 1477765 - Handle rejected Promises when inspecting an object;

https://reviewboard.mozilla.org/r/258938/#review266844

I think this isn't the right long-term solution, as explained in comment 28:
https://bugzilla.mozilla.org/show_bug.cgi?id=1477765#c28

But this is fine for the time being.
Attachment #8994361 - Flags: review?(jimb) → review+
(In reply to Jim Blandy :jimb from comment #28)
> If the rejected promise was created specifically as a result of trying to
> apply the accessor, as in the case in comment 0, then of course one should
> mark it as handled; the promise would never have existed if we hadn't tried
> to inspect the object.
> 
> But what if the accessor happens to be one that returns some user-provided
> value, and that value happens to be a promise that was recently rejected by
> the user's code? The developer tools must not mark such a promise as caught,
> because that would suppress the warning we've worked hard not to bury.

Unintentionally catching a user-generated promise and suppressing an error does sound bad. Though it's not actually clear to me when a user-provided getter would get to the point where we do `getter.call` and thus get a reference to the promise. For instance, this example doesn't evaluate the getter (not visible in the reps UI, and no exception appears):

({
  get foo() {
    return new Promise((_,r) => setTimeout(r, 1000))
  }
})
I was thinking of the case where a system-provided getter is returning a value that the user generates. In that case, a fully "legit" getter could still return a value that we should not mess with.
> Boris, what do you think of changing the API in this way?

You mean the web-visible API?  It's not going to change; promise-returning things are defined to convert all exceptions to rejected promises, after lots of arguments back and forth about what the behavior should be.

Note that "catching" a promise only affects whether it reports to the console, not whether other things calling catch() on it see it as rejected (they do).  And to suppress the console report you need to call catch() after the promise is rejected but before we have gone back out to the event loop; once we go to the event loop it will get reported.  So in practice, I expect there isn't a problem here: if the promise and its rejected state predates this whole operation, then it's already reported to console, if it's going to do it at all.
Flags: needinfo?(bzbarsky)
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5a3702fdbd1b
Handle rejected Promises when inspecting an object;r=jimb
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #32)
> > Boris, what do you think of changing the API in this way?
> Note that "catching" a promise only affects whether it reports to the
> console, not whether other things calling catch() on it see it as rejected
> (they do).  And to suppress the console report you need to call catch()
> after the promise is rejected but before we have gone back out to the event
> loop; once we go to the event loop it will get reported.  So in practice, I
> expect there isn't a problem here: if the promise and its rejected state
> predates this whole operation, then it's already reported to console, if
> it's going to do it at all.

Given this (and Comments 29 and 30), I went ahead and pushed this as-is. If we want to make a change in how we handle rejected promises for object previews we can do that in Bug 1478076 or a separate follow-up.
https://hg.mozilla.org/mozilla-central/rev/5a3702fdbd1b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: