Convert NS_GetCurrentThread uses in netwerk/

RESOLVED FIXED in Firefox 56

Status

()

Core
Networking
RESOLVED FIXED
7 months ago
6 months ago

People

(Reporter: billm, Assigned: billm)

Tracking

(Blocks: 1 bug)

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [necko-would-take])

Attachments

(1 attachment)

(Assignee)

Description

7 months ago
See https://bugzilla.mozilla.org/show_bug.cgi?id=1365096#c0.

This bug covers all uses in netwerk/.
Whiteboard: [necko-would-take]
(Assignee)

Comment 1

6 months ago
Created attachment 8875510 [details] [diff] [review]
patch

Just to give some context for this patch, here is the description from bug 1365096:

> This is a meta bug to cover the conversion:
> 
> 1. of NS_GetCurrentThread or NS_GetMainThread to GetCurrentThreadEventTarget or
> GetMainThreadEventTarget.
> 2. of thread equality checks to event target IsOnCurrentThread checks.
> 
> This will make things easier for the Quantum DOM project, since it will hide
> the fact that there can be multiple "main" threads that share a single main
> event loop.

The definitions of GetCurrentThreadEventTarget and nsISerialEventTarget can be found in bug 1361164.
Assignee: nobody → wmccloskey
Attachment #8875510 - Flags: review?(honzab.moz)
Comment on attachment 8875510 [details] [diff] [review]
patch

Review of attachment 8875510 [details] [diff] [review]:
-----------------------------------------------------------------

So, this depends on https://bugzilla.mozilla.org/attachment.cgi?id=8870204&action=diff right?  Would be good to list the bug# in the depends-on list.

LGTM, but I didn't check if there are more places to convert and also didn't much hunt for leaks/use-after-free (but I don't expect any from the nature of the api).

::: netwerk/base/nsInputStreamPump.cpp
@@ +132,5 @@
>          // Ensure OnStateStop is called on the main thread.
>          if (mState == STATE_STOP) {
>              nsCOMPtr<nsIEventTarget> mainThread = mLabeledMainThreadTarget
>                  ? mLabeledMainThreadTarget
> +                : do_AddRef(GetMainThreadEventTarget());

is the do_AddRef really necessary?  GetMainThreadEventTarget() returns plain nsIEventTarget* (surprisingly..) and here we assign to comptr.  So I don't see a need.
Attachment #8875510 - Flags: review?(honzab.moz) → review+

Comment 3

6 months ago
Pushed by wmccloskey@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/43fcf7ddd6ba
Convert NS_GetCurrentThread uses in netwerk/ (r=meyhemer)

Comment 4

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/43fcf7ddd6ba
Status: NEW → RESOLVED
Last Resolved: 6 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.