1412856 - replace direct usage NS_ERROR_DOM_TYPE_ERR in service workers with ErrorResult values with messages

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, enhancement, P3)

Description

[Ben Kelly [:bkelly, not reviewing]]

We have some places in service worker code where we throw or reject promises with NS_ERROR_DOM_TYPE_ERR.  This is ok per the spec, but these call sites don't provide a meaningful message to developers.  We should improve this code by using ErrorResult with a message.

To do this, however, we also need to make some improvements to ErrorResult to allow passing these more complex rejection values across IPC.  Some work in progress on that is in bug 1357463.

I had hoped to fix/avoid some of this in bug 1293277, but it was becoming a large project and we need to finish that work soon to unblock other work.

Comment 1

[Boris Zbarsky [:bzbarsky]]

> This is ok per the spec

I really doubt it.  It produces a DOMException with name == TypeError, not an actual TypeError.  Unfortunately, testharness just tests .name, not what the object really is.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #1)
> I really doubt it.  It produces a DOMException with name == TypeError, not
> an actual TypeError.  Unfortunately, testharness just tests .name, not what
> the object really is.

I was not aware of that.  FWIW, I don't think I made the problem any worse than it was before and have now added the primitives to fix it in the future.

Comment 3

[Boris Zbarsky [:bzbarsky]]

CopyableErrorResult might help here...

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1412856 part 1. Change ClientOpPromise to use a CopyableErrorResult for its rejection type. r=?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1412856 part 2. Stop using NS_ERROR_DOM_TYPE_ERR in Client::Navigate. r=?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1412856 part 3. Test for the right exception code for TypeErrors. r=?

I think the current code made us filter out all the carefully crafted TypeError
messages we might have reported...

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1412856 part 4. Make SourcePromise have CopyableErrorResult as its rejection type. r=?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1412856 part 5. Remove use of NS_ERROR_DOM_TYPE_ERR from Clients.openWindow. r=?

Some of these error messages are not very informative; better ones would be appreciated.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1412856 part 6. Add more useful error messages for serviceworker exceptions. r=?

For some of these I had to take a guess due to lack of familiarity with this
code, so careful review of the actual strings would be much appreciated.

There's still a bunch of code in dom/serviceworkers that constructs a
CopyableErrorResult from just an nsresult, but I don't understand that code well
enough to write good error messages.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Just to make sure it doesn't get lost: part 6 here still needs review.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0a3bf632a719
part 1.  Change ClientOpPromise to use a CopyableErrorResult for its rejection type.  r=dom-workers-and-storage-reviewers,sg?
https://hg.mozilla.org/integration/autoland/rev/aafd7c5a9d47
part 2.  Stop using NS_ERROR_DOM_TYPE_ERR in Client::Navigate.  r=dom-workers-and-storage-reviewers,perry?
https://hg.mozilla.org/integration/autoland/rev/1cac745bf19e
part 3.  Test for the right exception code for TypeErrors.  r=dom-workers-and-storage-reviewers,perry?
https://hg.mozilla.org/integration/autoland/rev/8c749ce01fb9
part 4.  Make SourcePromise have CopyableErrorResult as its rejection type.  r=dom-workers-and-storage-reviewers,sg?
https://hg.mozilla.org/integration/autoland/rev/c9d48dee0b74
part 5.  Remove use of NS_ERROR_DOM_TYPE_ERR from Clients.openWindow.  r=dom-workers-and-storage-reviewers,perry?
https://hg.mozilla.org/integration/autoland/rev/659060314d6d
part 6.  Add more useful error messages for serviceworker exceptions.  r=dom-workers-and-storage-reviewers,perry?

Comment 12

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/21557 for changes under testing/web-platform/tests

Comment 13

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 14

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/0a3bf632a719
https://hg.mozilla.org/mozilla-central/rev/aafd7c5a9d47
https://hg.mozilla.org/mozilla-central/rev/1cac745bf19e
https://hg.mozilla.org/mozilla-central/rev/8c749ce01fb9
https://hg.mozilla.org/mozilla-central/rev/c9d48dee0b74
https://hg.mozilla.org/mozilla-central/rev/659060314d6d

Comment 15

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot