This is a follow-up of bug 1335601 comment 3. The frequency of nsInputStreamReadyEvent is high according to the telemetry report in bug 1333984 which shall be labeled in priority.

Note: NS_NewPipe also implicitly creates Pipe(In|Out)putStream. When NS_New(In|Out)putStreamReadyEvent will be called when ::AsyncWait(..., nsIEventTarget* aTarget) is called. So, we could provide a proper {DocGroup|System}Group versioned EventTarget to run these (In|Out)putStreamEvent runnables.

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #1) > Note: NS_NewPipe also implicitly creates Pipe(In|Out)putStream. > When NS_New(In|Out)putStreamReadyEvent will be called when ::AsyncWait(..., > nsIEventTarget* aTarget) is called. > So, we could provide a proper {DocGroup|System}Group versioned EventTarget > to run these (In|Out)putStreamEvent runnables. As mentioned above, I will check where AsyncWait is called in netwerk folder [1]. [1] http://searchfox.org/mozilla-central/search?q=-%3EAsyncWait(&case=false&regexp=false&path=netwerk

Update my current findings below. Since all event targets used in NS_New(In|Out)putStreamReadyEvent are all passed from AsyncWait, we should check every place where AsyncWait is called. The search result of [1] could be summarized as: (1) Called from JS (2) netwerk/protocol/http/TunnelUtils.cpp (3) netwerk/protocol/file/nsFileChannel.cpp (4) dom/file/FileReader.cpp (5) dom/flyweb/HttpServer.cpp (6) dom/network/TCPSocket.cpp (7) dom/presentation/PresentationTCPSessionTransport.cpp (8) gfx/layers/LayerScope.cpp (9) ipc/glue/IPCStreamSource.cpp (10) netwerk/base/nsInputStreamPump.cpp (11) netwerk/base/nsPreloadedStream.cpp (12) netwerk/protocol/ftp/nsFtpConnectionThread.cpp (13) netwerk/protocol/ftp/nsFtpControlConnection.cpp (14) netwerk/protocol/http/nsHttpConnection.cpp (15) netwerk/protocol/http/nsHttpTransaction.cpp (16) netwerk/protocol/websocket/WebSocketChannel.cpp (17) xpcom/io/SlicedInputStream.cpp (18) xpcom/io/nsStreamUtils.cpp (19) netwerk/base/BackgroundFileSaver.cpp (20) netwerk/protocol/http/nsHttpConnectionMgr.cpp We need to file another bug to tackle (1). In JS, all AsyncWait users get a event target from nsIThreadManager, so maybe we have to add a labeled main thread target in nsIThreadManager.idl. IIRC, (2), (6), (7), (11), (12), (13), (14), (15), (16), and (20) all happen in parent process, so I think we can ignore them. (3), (17), and (19) are wrapped inside another AsyncWait, so we can also ignore these three places. (18) is inside the implementation of NS_AsyncCopy, which is on stream transport thread. We don't have to label it either. To summarize, we need to file some bugs for each directory below: - dom, for (4) and (5) - gfx, for (8) - ipc, for (9) @bevis, could you file the bugs as I mentioned above? Do you think this is a correct direction to go? Thanks. Note that this bug will be focused on (10), which is widely used in necko. [1] http://searchfox.org/mozilla-central/search?q=AsyncWait&case=false&regexp=false&path=

Thanks for breaking down this list! It really helps a lot. I've filed following bugs for tracking: (8) Bug 1359694 - Label nsIAsync(In|Out)putStream::AsyncWait() in LayerScopeWebSocketManager - Pref-off by default and is only used for debugging. (9) Bug 1359706 - Label IPCStreamSource::Callback (1) Bug 1359700 - Label nsIAsync(In|Out)putStream::AsyncWait() called from internal JS script And the following meta bugs that nsIAsync(In|Out)putStream::AsyncWait is invoked: (5) Bug 1359676 - Label runnables in dom/flyweb (4) Bug 1359686 - Label runnables in dom/file

Summary: - Add a new event target parameter in nsIInputStreamPump::Init - Add a new event target parameter in nsInputStreamPump::Create Thanks.

In addition to part1 patch, we also need other patches to pass the correct event target into nsIInputStreamPump::Init. That means we have to look into every place where nsIInputStreamPump is created. There are more than one way to create nsIInputStreamPump, so I list them below: [1] NS_newInputStreamPump - dom/fetch/Fetch.cpp - dom/workers/ScriptLoader.cpp - dom/workers/ServiceWorkerScriptCache.cpp - dom/worklet/Worklet.cpp - modules/libjar/nsJARChannel.cpp - modules/libjar/zipwriter/nsZipWriter.cpp - netwerk/protocol/wyciwyg/nsWyciwygChannel.cpp [2] nsInputStreamPump::Create - netwerk/base/nsBaseChannel.cpp - netwerk/protocol/about/nsAboutCacheEntry.cpp - netwerk/protocol/http/HttpChannelChild.cpp - netwerk/protocol/http/nsHttpChannel.cpp - toolkit/components/places/nsAnnoProtocolHandler.cpp [3] do_CreateInstance("@mozilla.org/network/input-stream-pump;1") - addon-sdk/source/lib/sdk/io/fs.js - addon-sdk/source/lib/sdk/io/stream.js - dom/network/TCPSocket.cpp - dom/presentation/provider/PresentationControlService.js - netwerk/base/NetUtil.jsm - toolkit/modules/Sntp.jsm - toolkit/modules/secondscreen/RokuApp.jsm Note that this bug will also cover files under netwerk. For other files, we need to file other follow-up bugs after part1 patch is landed. [1] http://searchfox.org/mozilla-central/search?q=NS_newInputStreamPump&case=false&regexp=false&path= [2] http://searchfox.org/mozilla-central/search?q=symbol:_ZN17nsInputStreamPump6CreateEPPS_P14nsIInputStreamlljjb&redirect=false [3] http://searchfox.org/mozilla-central/search?q=network%2Finput-stream-pump%3B1&case=false&regexp=false&path=

Summary: - Get an event target via |nsContentUtils::GetEventTargetByLoadInfo| and pass it in nsInputStreamPump::Create Note that for nsAboutCacheEntry.cpp and nsAnnoProtocolHandler.cpp, I am not sure they are happened in parent or child process, but I still do this for them.

Summary: - Pass an event target in nsIInputStreamPump::init - TCPSocket.cpp, PresentationTCPSessionTransport.cpp, and PresentationControlService.js are all in parent process, so using null is fine. Note that for the callers in js, we can only use null for now. We have to wait until bug 1359700 is fixed.

Comment on attachment 8863699 [details] [diff] [review] Part2: Pass event target in nsInputStreamPump::Create Review of attachment 8863699 [details] [diff] [review]: ----------------------------------------------------------------- r+ _with the comments fixed_ ::: netwerk/protocol/about/nsAboutCacheEntry.cpp @@ +485,5 @@ > return NS_OK; > } > > + nsCOMPtr<nsIEventTarget> target = nullptr; > + if (mChannel) { I believe there will always be a channel when you are here. Maybe just assert it, let's see how much will it crash ;) @@ +490,5 @@ > + nsCOMPtr<nsILoadInfo> loadInfo; > + mChannel->GetLoadInfo(getter_AddRefs(loadInfo)); > + nsCOMPtr<nsIEventTarget> target = > + nsContentUtils::GetEventTargetByLoadInfo(loadInfo, > + mozilla::TaskCategory::Other); uhh... nice bug here we have! you redeclare target. didn't warnings-as-errors config pref catch this?

Comment on attachment 8863700 [details] [diff] [review] Part3: Pass event target in nsInputStreamPump::init Review of attachment 8863700 [details] [diff] [review]: ----------------------------------------------------------------- r+ only for parts that use load info explicitly or I know the code. the rest should be reviewed by respective module peers. I want to make sure that there is no way to pass a valid target on some places you passes null. ::: dom/network/TCPSocket.cpp @@ +1001,5 @@ > nsresult rv; > mInputStreamPump = do_CreateInstance("@mozilla.org/network/input-stream-pump;1", &rv); > NS_ENSURE_SUCCESS(rv, rv); > > + rv = mInputStreamPump->Init(mSocketInputStream, -1, -1, 0, 0, false, nullptr); I think you have a window somewhere here. maybe ask :jdm, he maintains this code now. ::: dom/presentation/PresentationTCPSessionTransport.cpp @@ +281,5 @@ > if (NS_WARN_IF(NS_FAILED(rv))) { > return rv; > } > > + rv = mInputStreamPump->Init(mSocketInputStream, -1, -1, 0, 0, false, nullptr); I can't review this ::: dom/presentation/provider/PresentationControlService.js @@ +662,5 @@ > DEBUG && log("TCPControlChannel - create pump with role: " + > this._direction); // jshint ignore:line > this._pump = Cc["@mozilla.org/network/input-stream-pump;1"]. > createInstance(Ci.nsIInputStreamPump); > + this._pump.init(this._input, -1, -1, 0, 0, false, null); as well here ::: toolkit/modules/Sntp.jsm @@ +275,5 @@ > let outStream = transport.openOutputStream(0, 0, 0) > .QueryInterface(Ci.nsIAsyncOutputStream); > let inStream = transport.openInputStream(0, 0, 0); > > + pump.init(inStream, -1, -1, 0, 0, false, null); I also can't review this.

(In reply to Honza Bambas (:mayhemer) from comment #10) > Comment on attachment 8863699 [details] [diff] [review] > Part2: Pass event target in nsInputStreamPump::Create > > Review of attachment 8863699 [details] [diff] [review]: > ----------------------------------------------------------------- > > r+ _with the comments fixed_ > > ::: netwerk/protocol/about/nsAboutCacheEntry.cpp > @@ +485,5 @@ > > return NS_OK; > > } > > > > + nsCOMPtr<nsIEventTarget> target = nullptr; > > + if (mChannel) { > > I believe there will always be a channel when you are here. Maybe just > assert it, let's see how much will it crash ;) > When I was in Cache Storage Information page (about:cache?storage=disk&context=) and randomly clicked a key, the mChannel here is null. So, I got a crash below. Assertion failure: mChannel, at /Users/changkershaw/work/gecko/netwerk/protocol/about/nsAboutCacheEntry.cpp:489 #01: nsAboutCacheEntry::Channel::OnCacheEntryAvailable(nsICacheEntry*, bool, nsIApplicationCache*, nsresult)[/Users/changkershaw/work/gecko/obj-x86_64-apple-darwin16.5.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x488be2] #02: mozilla::net::CacheEntry::InvokeAvailableCallback(mozilla::net::CacheEntry::Callback const&)[/Users/changkershaw/work/gecko/obj-x86_64-apple-darwin16.5.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x41b518] #03: mozilla::net::CacheEntry::InvokeCallback(mozilla::net::CacheEntry::Callback&)[/Users/changkershaw/work/gecko/obj-x86_64-apple-darwin16.5.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x41eaee] #04: mozilla::net::CacheEntry::InvokeCallbacks(bool)[/Users/changkershaw/work/gecko/obj-x86_64-apple-darwin16.5.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x41e606] #05: mozilla::net::CacheEntry::InvokeCallbacks()[/Users/changkershaw/work/gecko/obj-x86_64-apple-darwin16.5.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x41c300] #06: mozilla::net::CacheEntry::Open(mozilla::net::CacheEntry::Callback&, bool, bool, bool)[/Users/changkershaw/work/gecko/obj-x86_64-apple-darwin16.5.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x41b31c] #07: mozilla::net::CacheEntry::AsyncOpen(nsICacheEntryOpenCallback*, unsigned int)[/Users/changkershaw/work/gecko/obj-x86_64-apple-darwin16.5.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x41b183] #08: mozilla::net::CacheStorage::AsyncOpenURI(nsIURI*, nsACString const&, unsigned int, nsICacheEntryOpenCallback*)[/Users/changkershaw/work/gecko/obj-x86_64-apple-darwin16.5.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x4714c2] #09: nsAboutCacheEntry::Channel::OpenCacheEntry()[/Users/changkershaw/work/gecko/obj-x86_64-apple-darwin16.5.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x488b49] #10: nsAboutCacheEntry::Channel::OpenCacheEntry(nsIURI*)[/Users/changkershaw/work/gecko/obj-x86_64-apple-darwin16.5.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x4884dd] #11: nsAboutCacheEntry::Channel::GetContentStream(nsIURI*, nsIInputStream**)[/Users/changkershaw/work/gecko/obj-x86_64-apple-darwin16.5.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x4882f1] #12: nsAboutCacheEntry::Channel::Init(nsIURI*, nsILoadInfo*)[/Users/changkershaw/work/gecko/obj-x86_64-apple-darwin16.5.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x4880fb] #13: nsAboutCacheEntry::NewChannel(nsIURI*, nsILoadInfo*, nsIChannel**)[/Users/changkershaw/work/gecko/obj-x86_64-apple-darwin16.5.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x487fa9] That's why I check if mChannel is null here. In addition, I think the function |nsAboutCacheEntry::Channel::WriteCacheEntryDescription| should be only called in parent process? If yes, we really don't have to get an event target here. But I am not 100% sure. Honza, could you confirm this? Thanks. > @@ +490,5 @@ > > + nsCOMPtr<nsILoadInfo> loadInfo; > > + mChannel->GetLoadInfo(getter_AddRefs(loadInfo)); > > + nsCOMPtr<nsIEventTarget> target = > > + nsContentUtils::GetEventTargetByLoadInfo(loadInfo, > > + mozilla::TaskCategory::Other); > > uhh... nice bug here we have! you redeclare target. didn't > warnings-as-errors config pref catch this? I did set warnings-as-errors in my mozconfig, but I have no idea why my compiler didn't complain about this. Sorry for this stupid mistake.

(In reply to Honza Bambas (:mayhemer) from comment #11) > Comment on attachment 8863700 [details] [diff] [review] > Part3: Pass event target in nsInputStreamPump::init > > Review of attachment 8863700 [details] [diff] [review]: > ----------------------------------------------------------------- > > r+ only for parts that use load info explicitly or I know the code. the > rest should be reviewed by respective module peers. I want to make sure > that there is no way to pass a valid target on some places you passes null. > Thanks for the review. I'll split this patch into other parts and ask someone to review.

(In reply to Kershaw Chang [:kershaw] from comment #12) > In addition, I think the function > |nsAboutCacheEntry::Channel::WriteCacheEntryDescription| should be only > called in parent process? If yes, we really don't have to get an event > target here. > But I am not 100% sure. Honza, could you confirm this? Thanks. Yes, this is a strictly parent thing. For now it's OK to let the if (mChannel) then. Still, aren't we having some prioritization of the main thread also on the parent process? > I did set warnings-as-errors in my mozconfig, but I have no idea why my > compiler didn't complain about this. > Sorry for this stupid mistake. No problem. Redeclarations in sub-blocks are silently eaten by AFAIK any compiler. One reason I enforce declaring |nsresult rv| as the first and only declaration of rv in a function. You may easily get caught.

(In reply to Honza Bambas (:mayhemer) from comment #14) > (In reply to Kershaw Chang [:kershaw] from comment #12) > > In addition, I think the function > > |nsAboutCacheEntry::Channel::WriteCacheEntryDescription| should be only > > called in parent process? If yes, we really don't have to get an event > > target here. > > But I am not 100% sure. Honza, could you confirm this? Thanks. > > Yes, this is a strictly parent thing. For now it's OK to let the if > (mChannel) then. > > Still, aren't we having some prioritization of the main thread also on the > parent process? > No, I don't think we have to label runnables in parent process right now. But I am not sure if we need it some day. :btseng, do you think we have to care runnables in parent now? Do we have a plan for labeling also happening in parent? Thanks.

No, there is no plan to label runnables in parent process. We only want to schedule the tasks in multiple web contents by prioritizing tasks in content process. (e10s-enabled)

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #16) > No, there is no plan to label runnables in parent process. > We only want to schedule the tasks in multiple web contents by prioritizing > tasks in content process. (e10s-enabled) Thanks! I'll keep nsAboutCacheEntry.cpp unchanged since we don't have to label it in parent process.

Summary: - Pass a null event target to nsInputStreamPump::Init If I am right, TCPSocket::CreateInputStreamPump should be only called in parent process. If so, I think we can just pass nullptr here since we don't need to label runnables in parent process.

@SC, could you take a look at this patch? I think the two files in this patch should be only used in parent process, so I think we can just pass nullptr here? Thanks.

Hi Bevis, I gonna need your signature here. Since we don't have access to labeled event targets in js right now, I can only pass null when calling nsInputStreamPump::Init in js. Thanks.

Comment on attachment 8865738 [details] [diff] [review] Part 5: Pass a null event target for calling nsInputStreamPump::Init Review of attachment 8865738 [details] [diff] [review]: ----------------------------------------------------------------- Yes, these codes only run in parent process so passing nullptr is fine.

Comment on attachment 8865740 [details] [diff] [review] Part 6: Pass a null event target when calling nsInputStreamPump::Init in JS Review of attachment 8865740 [details] [diff] [review]: ----------------------------------------------------------------- If "aMainThreadTarget" in Init() is nullable, it seems that we can mark it as [optional] in the idl declaration instead of making change in these .js files. How do you think?

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #22) > Comment on attachment 8865740 [details] [diff] [review] > Part 6: Pass a null event target when calling nsInputStreamPump::Init in JS > > Review of attachment 8865740 [details] [diff] [review]: > ----------------------------------------------------------------- > > If "aMainThreadTarget" in Init() is nullable, it seems that we can mark it > as [optional] in the idl declaration instead of making change in these .js > files. > How do you think? Good idea! Thanks for point this out. I forgot that we can add [optional] in idl. Honza, what do you think? I'd like to modify the idl as below. * a labeled main therad event target. */ void init(in nsIInputStream aStream, in long long aStreamPos, in long long aStreamLen, in unsigned long aSegmentSize, in unsigned long aSegmentCount, in boolean aCloseWhenDone, - in nsIEventTarget aMainThreadTarget); + [optional] in nsIEventTarget aMainThreadTarget); /** * asyncRead causes the input stream to be read in chunks and delivered * asynchronously to the listener via OnDataAvailable. * * @param aListener * receives notifications. * @param aListenerContext

(In reply to Kershaw Chang [:kershaw] from comment #23) > (In reply to Bevis Tseng[:bevistseng][:btseng] from comment #22) > > Comment on attachment 8865740 [details] [diff] [review] > > Part 6: Pass a null event target when calling nsInputStreamPump::Init in JS > > > > Review of attachment 8865740 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > If "aMainThreadTarget" in Init() is nullable, it seems that we can mark it > > as [optional] in the idl declaration instead of making change in these .js > > files. > > How do you think? > > Good idea! > Thanks for point this out. I forgot that we can add [optional] in idl. > > Honza, what do you think? I'd like to modify the idl as below. > > * a labeled main therad event target. > */ > void init(in nsIInputStream aStream, > in long long aStreamPos, > in long long aStreamLen, > in unsigned long aSegmentSize, > in unsigned long aSegmentCount, > in boolean aCloseWhenDone, > - in nsIEventTarget aMainThreadTarget); > + [optional] in nsIEventTarget aMainThreadTarget); > > /** > * asyncRead causes the input stream to be read in chunks and delivered > * asynchronously to the listener via OnDataAvailable. > * > * @param aListener > * receives notifications. > * @param aListenerContext Agree! Thanks.

Summary: - Add [optional] for mainThreadTarget

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e83cf5af482b Part1: Pass a labeled event target when initializing nsInputStreamPump, r=mayhemer https://hg.mozilla.org/integration/mozilla-inbound/rev/6823d1c41c5f Part2: Pass an event target when calling nsInputStreamPump::Create, r=mayhemer https://hg.mozilla.org/integration/mozilla-inbound/rev/49c26fdd9b8c Part3: Pass an event target at places where nsIInputStream::init is called., r=mayhemer https://hg.mozilla.org/integration/mozilla-inbound/rev/41678812d3cc Part4: Pass a null event target in nsInputStreamPump::Init, r=jdm https://hg.mozilla.org/integration/mozilla-inbound/rev/e1b7cdaed4a1 Part5: Pass a null event target, r=schien

https://hg.mozilla.org/mozilla-central/rev/e83cf5af482b https://hg.mozilla.org/mozilla-central/rev/6823d1c41c5f https://hg.mozilla.org/mozilla-central/rev/49c26fdd9b8c https://hg.mozilla.org/mozilla-central/rev/41678812d3cc https://hg.mozilla.org/mozilla-central/rev/e1b7cdaed4a1