Label the use of NS_NewInputStreamPump in CacheScriptLoader.

RESOLVED FIXED in Firefox 56

Status

()

Core
DOM: Workers
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: bevis, Assigned: bevis)

Tracking

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment)

NS_NewInputStreamPump() is used in CacheScriptLoader on the main thread. [1]
Besides, CacheScriptLoader is associated with a WorkerPrivate when creating [2], so we could use the WorkerPrivate::MainThreadEventTarget() for the labeling of NS_NewInputStreamPump().


[1] http://searchfox.org/mozilla-central/rev/17ebac68112bd635b458e831670c1e506ebc17ad/dom/workers/ScriptLoader.cpp#1704
[2] http://searchfox.org/mozilla-central/rev/17ebac68112bd635b458e831670c1e506ebc17ad/dom/workers/WorkerPrivate.cpp#5157
Created attachment 8882777 [details] [diff] [review]
(v1) Patch: Label the use of NS_NewInputStreamPump in CacheScriptLoader.

Label the use of NS_NewInputStreamPump() with WorkerPrivate::MainThreadEventTarget().

Treeherder result looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=92425608bb35c170f018c01a1da19149a61523a6
Attachment #8882777 - Flags: review?(bkelly)
Comment on attachment 8882777 [details] [diff] [review]
(v1) Patch: Label the use of NS_NewInputStreamPump in CacheScriptLoader.

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

Note, this will cause this data to load through a ThrottledEventQueue when it did not previously.  I think this is ok as I'd rather delay worker script load than jank the main thread if its busy.
Attachment #8882777 - Flags: review?(bkelly) → review+
(In reply to Ben Kelly [reviewing, but slowly][:bkelly] from comment #2)
> Note, this will cause this data to load through a ThrottledEventQueue when
> it did not previously.  I think this is ok as I'd rather delay worker script
> load than jank the main thread if its busy.

I have the same concern when modifying this. (I should rise my concern directly when asking the review. :\)
Thanks for your clarification on this! :)
In the future we may want to see if the data load can be redirected to a non-main thread target now that e10s necko supports that.

Comment 5

a year ago
Pushed by btseng@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d384edb68e6
Label the use of NS_NewInputStreamPump in CacheScriptLoader. r=bkelly

Comment 6

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3d384edb68e6
Status: NEW → RESOLVED
Last Resolved: a year 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.