1220007 - make intercepted channel's use of ConsoleReportCollector less fragile

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

The code in P10 of bug 1215140 depends on the intercepted channels lifecycle to be threadsafe.  We should make this a bit more robust.

Possible plan:

1) Store a ConsoleReporterCollector in the intercepted channel itself that survives as long as necessary.
2) Add pending reports to the that collector on the worker thread.
3) When the InterceptedChannel is back on the main thread then send the logs to the nsIChannel's collector.

This probably means adding an nsIConsoleReporterCollector::SwapConsoleReports() methods.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Allow ConsoleReportCollectors to flush to another collector. r=bz

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Make InterceptedChannel's collect logs locally and only flush to nsIChannel on main thread r=bz

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8681361 [details] [diff] [review]
P1 Allow ConsoleReportCollectors to flush to another collector. r=bz

r=me and special bonus points for scoping the lock so that foo->FlushConsoleReports(foo) (dumb as that is) will not deadlock.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8681362 [details] [diff] [review]
P2 Make InterceptedChannel's collect logs locally and only flush to nsIChannel on main thread r=bz

Why do we need the changes to the IDL file?  What was wrong with continuing to return the non-addrefed pointer?

r=me modulo that, though I'm taking it on faith that you put those flush calls in all the right places.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Boris Zbarsky [:bz] from comment #4)
> Why do we need the changes to the IDL file?  What was wrong with continuing
> to return the non-addrefed pointer?

The intercepted channel is a main thread object.  In theory it could go away after get the handle the nsIConsoleReportCollector.  Hold a strong ref to the collector seemed the most thread safe to me.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Ah, ok.  May be worth documenting.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Boris Zbarsky [:bz] from comment #6)
> Ah, ok.  May be worth documenting.

Honestly, its just a little bit of extra protection.  The current code should prevent that from happening, but this is just trying to make it more bullet proof to future changes.  I don't want to confuse things by warning against something that doesn't currently happen.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c253b6bd00b1
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8d75be11275

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c253b6bd00b1
https://hg.mozilla.org/mozilla-central/rev/d8d75be11275

Comment 10

[Carsten Book [:Tomcat]]

sorry had to back this out. When this changes got merged back to the integration trees i noticed this is causing somehow a bustage on b2g-emulator L debug builds like https://treeherder.mozilla.org/logviewer.html#?job_id=3245188&repo=b2g-inbound and we didn't saw this before because emulator L debug builds seem to run only there.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Backouts:
https://hg.mozilla.org/mozilla-central/rev/59a6ad6a921f
https://hg.mozilla.org/mozilla-central/rev/aeabfab3ea4d

and:

https://hg.mozilla.org/integration/mozilla-inbound/rev/59a6ad6a921f
https://hg.mozilla.org/integration/mozilla-inbound/rev/aeabfab3ea4d

Its very strange that the compiler in b2g-inbound failed, while m-c and m-i did not.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Try against b2g-inbound:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=42f600f27ce6

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Make InterceptedChannel's collect logs locally and only flush to nsIChannel on main thread r=bz

Updated patch to CLOBBER.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9553150987e5
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/508d22157bca

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9553150987e5
https://hg.mozilla.org/mozilla-central/rev/508d22157bca

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/9553150987e5
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/508d22157bca

Comment 17

[Carsten Book [:Tomcat]]

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8681361 [details] [diff] [review]
P1 Allow ConsoleReportCollectors to flush to another collector. r=bz

Approval request for P1 and P2.  All patches.

Approval Request Comment
[Feature/regressing bug #]: Service workers
[User impact if declined]: I need this in order to uplift bug 1217909 and other dependent bugs.
[Describe test coverage new/current, TreeHerder]: Local testing and mochitests.
[Risks and why]: Minimal.  Only affects service workers.
[String/UUID change made/needed]: None

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Ritu, sorry for the direct NI, but if we could uplift this older patch it would make getting bug 1217909 on aurora much easier.  I'd like to uplift tomorrow if I can.

Thanks!

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8681361 [details] [diff] [review]
P1 Allow ConsoleReportCollectors to flush to another collector. r=bz

This has been on Nightly for almost two weeks, seems safe to uplift to Aurora44.

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Ben Kelly [:bkelly] from comment #19)
> Ritu, sorry for the direct NI, but if we could uplift this older patch it
> would make getting bug 1217909 on aurora much easier.  I'd like to uplift
> tomorrow if I can.
> 
> Thanks!

No worries. Done!

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

Note for the sheriffs, I will uplift this myself along with bug 1217909.

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f0b7aa7305ff
https://hg.mozilla.org/releases/mozilla-aurora/rev/3f63e0a21969

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/f0b7aa7305ff
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/3f63e0a21969