1384294 - label HttpChannelChild::SetIsTrackingResource

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement)

Description

[Nathan Froyd [:froydnj]]

This runnable comes in fairly high on the list, according to telemetry.  Which is depressing!

It's not clear to me what the right thing to do here is.  HttpBackgroundChannelChild::RecvNotifyTrackingResource() says that the notification is to be handled "as soon as possible", but then HttpChannelChild::ProcessNotifyTrackingResource allocates and dispatches an event to set a boolean flag?  Couldn't we just set the flag straightaway here?

If not, HttpChannelChild::SetEventTarget attempts to associate an event target with `this`, but it's not clear to me that we could retrieve that event target in ProcessNotifyTrackingResource and use that for dispatching.  If we can't do that, I don't see a good way to send this event to anything but SystemGroup.

Jason, can you provide some guidance here?

Comment 1

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Hmm, maybe I should replace the |GetMainThreadEventTarget| with |GetNeckoTarget|, which is the associated with the corresponding docgroup/tabgroup.

Comment 2

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Making |HttpBaseChannel::mIsTrackingResource| atomic and call |SetIsTrackingResource()| directly in HttpChannelChild::ProcessNotifyTrackingResource is also doable. Not sure why I didn't take this solution at the first place while writing the PBg-Http code.

Comment 3

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Bug 1384294 - replace GetMainThreadEventTarget with GetNeckoTarget in HttpChannelChild.

GetMainThreadEventTarget doesn't label runnable with correct doc group or tab group.
In HttpChannelChild we should replace it with |GetNeckoTarget| in general.
By making mIsTrackingResource atomic, HttpChannelChild::ProcessNotifyTrackingResource can be
further changed to do direct function call without dispatching additional runnable on main thread.

Review commit: https://reviewboard.mozilla.org/r/161360/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/161360/

Comment 4

[Kershaw Chang [:kershaw]]

Thanks for working on this, SC.

I thinks we can remove the ni to Jason.

Comment 5

[Honza Bambas (:mayhemer)]

Comment on attachment 8890239 [details]
Bug 1384294 - replace GetMainThreadEventTarget with GetNeckoTarget in HttpChannelChild.

https://reviewboard.mozilla.org/r/161360/#review167398

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d42bfe2b2e63
replace GetMainThreadEventTarget with GetNeckoTarget in HttpChannelChild. r=mayhemer

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d42bfe2b2e63