1269400 - NS_ERROR_UNEXPECTED thrown by Web IDL callbacks from unloaded documents is not very informative

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Reduced STRs from a bug :pehrsons found:

1. Open URL.
2. Wait 12 seconds for GC.

Expected result: no output.
Actual result: NS_ERROR_UNEXPECTED (unknown) in web console.

Seems to come from here:
> Thread 1Queue : com.apple.main-thread (serial)
> #0	0x0000000107385e64 in mozilla::dom::AsyncErrorReporter::AsyncErrorReporter(xpc::ErrorReport*) at /Users/Jan/moz/mozilla-central/dom/base/nsJSEnvironment.h:195
> #1	0x00000001074a45e0 in mozilla::dom::Promise::MaybeReportRejected() at /Users/Jan/moz/mozilla-central/dom/promise/Promise.cpp:2571
> #2	0x00000001074ac7b9 in mozilla::dom::Promise::MaybeReportRejectedOnce() at /Users/Jan/moz/mozilla-central/obj-x86_64-apple-darwin12.2.1-debug/dist/include/mozilla/dom/Promise.h:406
> #3	0x000000010749af46 in mozilla::dom::Promise::cycleCollection::Unlink(void*) at /Users/Jan/moz/mozilla-central/dom/promise/Promise.cpp:398
> #4	0x00000001031302af in nsCycleCollector::CollectWhite() at /Users/Jan/moz/mozilla-central/xpcom/base/nsCycleCollector.cpp:3324
> #5	0x0000000103131681 in nsCycleCollector::Collect(ccType, js::SliceBudget&, nsICycleCollectorListener*, bool) at /Users/Jan/moz/mozilla-central/xpcom/base/nsCycleCollector.cpp:3670
> #6	0x00000001031332ae in nsCycleCollector_collectSlice(js::SliceBudget&, bool) at /Users/Jan/moz/mozilla-central/xpcom/base/nsCycleCollector.cpp:4157
> #7	0x00000001052a312e in nsJSContext::RunCycleCollectorSlice() at /Users/Jan/moz/mozilla-central/dom/base/nsJSEnvironment.cpp:1550
> #8	0x00000001052a34bf in ICCTimerFired(nsITimer*, void*) at /Users/Jan/moz/mozilla-central/dom/base/nsJSEnvironment.cpp:1608
> ...

Comment 1

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Side-question: Is it correct behavior for the setTimeout promise to never resolve? Here's the code:

    onload = () => iframe.contentDocument.foo().then(() => {
      iframe.srcdoc = "";
      return new Promise(r => setTimeout(r, 1000));
    })
    .then(() => console.log("success"))
    .catch(e => console.error(e));

We were bitten by it.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> Actual result: NS_ERROR_UNEXPECTED (unknown) in web console.

Presumably someone rejected the Promise with NS_ERROR_UNEXPECTED.  When JS promises land the behavior here might change, of course.

Is this bug report about the fact that the promise got rejected?  About the fact that we reported the rejection to the console (as in, was this expected to be a promise that was caught)?  Something else?

In general, the behavior of promises in pages that have been navigated away from is not defined in any specifications.... I believe we basically prevent their callbacks from being called, so pages can't keep chewing up CPU resources once they have been navigated away from.

> Is it correct behavior for the setTimeout promise to never resolve?

Do you mean the "new Promise(r => setTimeout(r, 1000))?  Or do you mean the return value of that foo().then() call?  I would expect the former to resolve and the latter has undefined behavior per above.

Comment 3

[Andreas Pehrson [:pehrsons]]

(In reply to Boris Zbarsky [:bz] from comment #2)
> > Actual result: NS_ERROR_UNEXPECTED (unknown) in web console.
> 
> Presumably someone rejected the Promise with NS_ERROR_UNEXPECTED.  When JS
> promises land the behavior here might change, of course.
> 
> Is this bug report about the fact that the promise got rejected?  About the
> fact that we reported the rejection to the console (as in, was this expected
> to be a promise that was caught)?  Something else?

The promise didn't get rejected. It seems like internal code fails to get the promise and prints a (vague) error to the console instead. It would be great if the promise was rejected when cleaned up, or if the printed message was more useful, since a user would typically expect the promise to resolve, reject with some additional information, or at worst print some additional information to the console so the user understands why it didn't work. Now it does the latter, but "additional information" is not quite there.

> In general, the behavior of promises in pages that have been navigated away
> from is not defined in any specifications.... I believe we basically prevent
> their callbacks from being called, so pages can't keep chewing up CPU
> resources once they have been navigated away from.
> 
> > Is it correct behavior for the setTimeout promise to never resolve?
> 
> Do you mean the "new Promise(r => setTimeout(r, 1000))?  Or do you mean the
> return value of that foo().then() call?  I would expect the former to
> resolve and the latter has undefined behavior per above.

It's as you expect.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> The promise didn't get rejected.

Which "the promise"?  There are 6 or more promises in this testcase.

In any case, there is totally a Promise being rejected with NS_ERROR_UNEXPECTED, and it's a promise that's not at all obvious.

In particular, when the timer fires we resolve the Promise created by this bit: new Promise(r => setTimeout(r, 1000)).  This promise has a handler, which is a callback to resolve the promise created by the contentDocument.foo().then() call.  It attempts to call this handler, and the handler throws NS_ERROR_UNEXPECTED.  That's because we disallow calls to Web IDL callbacks backed by a function that comes from a non-active document; attempts to call such a thing throw NS_ERROR_UNEXPECTED.

OK, but where did this handler come from to start with?  When the function we passed to contentDocument.foo().then() returned a Promise, we saw that the return value is a thenable and called its then() method, per spec, passing in a function that resolves the next promise in the chain when the returned thenable resolves.  This has two effects: adding that function as a callback _AND_ creating a new Promise, which is never exposed to script anywhere.  It's basically this setup:

  var thenable = returnValue;
  var p = thenable.then(resolveMyNextPromise, rejectMyNextPromise); // p is never exposed to script

Now in this case resolveMyNextPromise ends up throwing NS_ERROR_UNEXPECTED because we don't allow the call.  This rejects p in the usual way.  Since p was never exposed to script, it has no handlers, and when it dies we treat it as an unhandled rejection.

Now the fact that p exists at all is really unfortunate, but pretty much required by the ES spec.  :(

I'm still trying to figure out what this bug report is asking for.  Is it that we add better console messaging for the "can't call things in the unloaded document" thing?  Right now it just throws an exception without logging anything.  We could change it to either log directly or throw a more informative exception....

Comment 5

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Thanks for the thorough walk-through. I assumed you'd want a bug filed on an NS_ERROR_UNEXPECTED. When people see NS_ERROR_UNEXPECTED in WebRTC they file bugs, and we either provide a better error or make it disappear. Legitimate cases are new to me.

How about:

  p.catch(() => {});

Since this is now expected, and no-one knows or cares about p, NS_ERROR_UNEXPECTED seems like noise.

(In reply to Boris Zbarsky [:bz] from comment #2)
> In general, the behavior of promises in pages that have been navigated away
> from is not defined in any specifications.... I believe we basically prevent
> their callbacks from being called, so pages can't keep chewing up CPU
> resources once they have been navigated away from.

wfm.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> I assumed you'd want a bug filed on an NS_ERROR_UNEXPECTED.

I definitely do if it's being thrown to content.  If it's just being reported to the console like this... probably still worth a bug report.  That said, the "thrown to content" case is pretty simple to produce here; I'll attach a testcase.   Sounds like we should focus on making that NS_ERROR_UNEXPECTED a more informative exception.

> How about:
>  p.catch(() => {});

I wish we could.. but that would be page-observable in general: "p" might have "catch" as an accessor property, or be a Proxy, or whatever.

> Since this is now expected, and no-one knows or cares about p

That's only true if the thenable is an actual Promise and its "then" method is the original Promise.prototype.then and it's not a subclass that overrides Symbol.species, and probably a few other assumptions.  Basically, true in the common case, but you can craft returnValue objects for which it's totally false and "p" is in fact quite observable by the web page, as are any property accesses on it, and the page can add handlers to it, etc.  And it's not clear to me that such a returnValue object can even be detected reliably in a side-effect-free way...

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase (though this one might stop showing the problem with spidermonkey promises)

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase not involving Promise at all

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Provide better error messages for the cases when a CallSetup fails and we hence fail to execute a Web IDL callback

Comment 10

[Boris Zbarsky [:bzbarsky]]

With that patch, the console log on the original testcase for this bug says:

  NotSupportedError: Refusing to execute Web IDL callback from window whose document is no longer active.

which is still moderately confusing because it's not obvious where the Web IDL callback is, exactly, but....

Comment 11

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Is there a precedent for talking about "Web IDL" to content? Not sure it means anything to web devs. Would "function" work? The important bit seems to be about the document that went away.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Hmm.  We could do that, yes.  So just s/Web IDL callback/function/ in those cases?  The incumbent global message won't make much sense that way, but that one is not very likely to be hit anyway.

Comment 13

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

wfm.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ed75e9e62c0

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0ed75e9e62c0