1604543 - ServiceWorkerManager::ReportToAllClients needs to be updated for sw-e10s [syntax errors in script evaluation receive the reported-to-content MSG_SW_INSTALL_ERROR but not the syntax errors]

Status: NEW

(Core :: DOM: Service Workers, defect, P2)

Description

[:Harald Kirschner :digitarald]

Attachment: Chrome vs Firefox

STR:

• Load a SW with a syntax error

AR: Generic error message TypeError: ServiceWorker script at X for scope Y encountered an error during installation.

ER: Syntax error reported for correct file.

Comment 1

[:Harald Kirschner :digitarald]

Andrew, would this be on DOM team or JS?

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

This is one of those: "devtools could technically do this right now without ServiceWorker backend changes, but the right thing is a ServiceWorker backend change".

I think the syntax errors are getting emitted in the parent process right now as a ConsoleEvent by ConsoleUtils::ReportForServiceWorkerScopeInternal and are emitting stuff to "@mozilla.org/consoleAPI-storage;1" with an inner window ID of "ServiceWorker" and an outer window ID of the ServiceWorker scope.

I see handling for this case in ConsoleAPIListener.isMessageRelevant.

I assume the situation is that this worked under child-intercept because the ServiceWorkerManager emitting the console message, the listening actor, and the window lived in the same process. Now the ServiceWorkerManager and the console message live in the parent and the listening actor and its window live in a content process. Unless the devtools actors are propagating the messages?

It seems like the place where the semantics mismatch is happening is at ServiceWorkerManager::ReportToAllClients. Under parent-intercept, the thing ServiceWorkers should be doing is should be finding all the matching clients and sending the error down to them where they can report it in their process. This seems appropriate given the idea that a user should be able to open devtools after a bad thing has happened and see the bad thing. This would presumably require no devtools changes, although I haven't 100% processed what's going on.

I think we probably also need to add test coverage, as it's likely we don't have it and that's how we missed it as part of the sw-e10s devtools steps.

Unfortunately, given the imminent holidays, this might not make it into 73 unless it gets uplifted early in the cycle.

Comment 3

[:Harald Kirschner :digitarald]

This is one of those: "devtools could technically do this right now without ServiceWorker backend changes, but the right thing is a ServiceWorker backend change".

Perry, given Andrew's comment, do you think this is more trivial do solve now?

Comment 4

[Perry Jiang [:perry] [no longer employee, use ni?]]

It seems easier to solve yeah, but I won't be able to get to it unfortunately.