Closed Bug 1220007 Opened 4 years ago Closed 4 years ago

make intercepted channel's use of ConsoleReportCollector less fragile

Categories

(Core :: DOM: Service Workers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox43 --- wontfix
firefox44 --- fixed
firefox45 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
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.
Attachment #8681361 - Flags: review?(bzbarsky) → review+
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.
Attachment #8681362 - Flags: review?(bzbarsky) → review+
(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.
Ah, ok.  May be worth documenting.
(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.
https://hg.mozilla.org/mozilla-central/rev/c253b6bd00b1
https://hg.mozilla.org/mozilla-central/rev/d8d75be11275
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
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.
Flags: needinfo?(bkelly)
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.
Status: RESOLVED → REOPENED
Flags: needinfo?(bkelly)
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/9553150987e5
https://hg.mozilla.org/mozilla-central/rev/508d22157bca
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
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
Attachment #8681361 - Flags: approval-mozilla-aurora?
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!
Flags: needinfo?(rkothari)
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.
Flags: needinfo?(rkothari)
Attachment #8681361 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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!
Note for the sheriffs, I will uplift this myself along with bug 1217909.
You need to log in before you can comment on or make changes to this bug.