1498705 - Document use of mutex in ConsoleReportCollector

Status: NEW

(Core :: DOM: Core & HTML, enhancement, P3)

Description

[Randell Jesup [:jesup] (needinfo me)]

In a 1min browsing session, I saw ~2K ConsoleReportCollector mutexes created and destroyed, with non contention and few lock()s (all either 0 locks or 1 lock).  

Also, all seem to be accessed on MainThread, always.

I've done a run with MOZ_RELEASE_ASSERTs on NS_IsMainThread(), with no crashes.

We should consider removing the assertion, analyzing if there is (still?) a reason for it, and replacing with MOZ_ASSERT()s.

They seem to be created from the style Loader, ScriptLoader, HttpBaseChannel and InterceptedChannel, and the DOM Fetch driver.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Convert ConsoleReportCollector to MOZ_RELEASE_ASSERT() NOT FOR CHECKIN

(this might be landable using MOZ_ASSERT()).

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=714d464764786e2b60831df1e6b2705fe7489c82 with several other mutex patches

Comment 3

[Andrew McCreight [:mccr8]]

It looks like this is related to service workers, so you'd have to test it out on a site that uses those. Andrew, are you familiar with this class? I can't imagine bkelly added the locks for no reason.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

They are used on DOMWorker threads; I suggest a specialized subclass for that usage perhaps.

Comment 5

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

Is this an immediate performance concern?  In practice your assertions will hold right now because:
a) We have to bounce so many things to the main thread for principal/URL reasons, but that's something we're trying to fix.
b) Your assertions won't catch simultaneous corrupt manipulation of the underlying array data-structure which I think is what the mutex is trying to protect against.
b2) I'm a little worried about additional things becoming possible once we turn on JS streams imminently...  Right now the channel hand-off is transactionally atomic ("here's an immutable response"), but with streams turned on, the SW can continuously pump data into the necko channel.
c) It seems likely you didn't test with sites using ServiceWorker interception.

We should be able to clean up at least the ServiceWorker-related uses of this API as part of bug 1496997.


Context:

The general idea of nsIConsoleReportCollector AIUI is that it's a bucket that we put console reports in from any thread that's servicing a channel, such as ServiceWorker threads servicing an intercepted "fetch" request or maybe weird necko threads.  The bucket gets emptied into either another nsIConsoleReportCollector or dumped to the console when it finally researches a window and the main thread.  The API likely partially suffers from being a bit generic in nature with ill-defined thread ownership semantics.

We're changing the way the ServiceWorker provides feedback as part of the ServiceWorkers e10s overhaul, with the hand-off of this information occurring in the parent process, at which point it will be relayed back down to the child.  This will have a much more clear ownership hand-off for the info.


Tests:

The tests in dom/serviceworkers should exercise the code paths.  In particular, some of those that use error_reporting_helpers.js[1].

1: https://searchfox.org/mozilla-central/search?q=error_reporting_helpers&path=

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

In fact, when DOM Workers get used my assertions will fail; which I found when IIRC trying to look at youtube.

Uncontended mutexes are cheap to acquire, and these are uncontended.  And (ignoring DEBUG) creation and deletion of mutexes is also cheap (if they're part of something you're creating anyways).  So no, I don't think this is an actual perf bottleneck.  Of course, if it's not actually needed then let's get rid of it (and maybe assert we don't break this later).  

Some comments as to why it's there would be good; revectoring this bug

Comment 7

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

YouTube installs a no-fetch ServiceWorker even if you don't enable push notification, I think.  This will end up controlling all youtube pages and any workers they spawn so the interception path will be triggered.  (We just won't wake up the ServiceWorker or dispatch a "fetch" event at it.  Yes, this adds latency.)

Comment 8

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

Er, and you can easily verify if this is impacting you by evaluating "navigator.serviceWorker.controller" via devtools.  Note that we don't expose the ServiceWorkerContainer on the Worker so trying it there won't work even if the Worker is controlled.

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

I was just agreeing that my assertions aren't reasonable, and that a mutex is needed.  It's not contended, so there isn't a big issue around it, but it could use some comments about why it is needed.

Comment 10

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

Agreed about the need for comments, this area of the code is very confusing.  (And in theory I have a handle on what's going on and it still took me quite some time to refresh my understanding.)

Aside: I realize my phrasing of "easily verify" may have been poor and come off as flippant.  This is not something I'd expect people to know.  Likewise, the fact that a push subscription can cause a page to be marked as controlled by a ServiceWorker even though the ServiceWorker doesn't intercept fetch events for the page is even more non-obvious.  And our devtools story is bad for ServiceWorkers right now, so I just wanted to let you/other readers know that "navigator.serviceWorker.controller" is one of the few bright spots in figuring out what's going on when ServiceWorkers are involved without having to spend an hour in gdb/clang.  (And we are of course working on all of this stuff; hopefully many good announcements to make on things being sane at the All Hands.)