1024388 - Allow JAR channels to be retargeted to a different thread.

Status: RESOLVED FIXED

(Core :: Networking: JAR, defect)

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 1: Attempt to retarget the HTML parser's request regardless of the stream type

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 2: Implement thread retargeting for JAR channels

My try run is not yet complete, but it doesn't look like anything depends on the progress listener notifications ...

What's the correct way to reimplement that?  Should I just dispatch a runnable to call OnProgressChange to the main thread?

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://tbpl.mozilla.org/?tree=Try&rev=5b63520cfd4a

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8439060 [details] [diff] [review]
Part 1: Attempt to retarget the HTML parser's request regardless of the stream type

r=me assuming the warning doesn't fire too much.  And maybe s/delivery/HTML data delivery/?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8439062 [details] [diff] [review]
Part 2: Implement thread retargeting for JAR channels

> Should I just dispatch a runnable to call OnProgressChange to the main thread?

I would assume yes.  That's exactly what httpchannel does in OnDataAvailable:

5297         if (NS_IsMainThread()) {
5298             OnTransportStatus(nullptr, transportStatus, progress, progressMax);
5299         } else {
5300             nsresult rv = NS_DispatchToMainThread(
5301                 new OnTransportStatusAsyncEvent(this, transportStatus,
5302                                                 progress, progressMax));
5303             NS_ENSURE_SUCCESS(rv, rv);
5304         }

nsHttpChannel::OnTransportStatus does various stuff including calling mProgressSink->OnProgress.  OnTransportStatusAsyncEvent just invokes OnTransportStatutus on its first arg.

Document the lifetime guarantees on mRequest, please.

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 1: Attempt to retarget the HTML parser's request regardless of the stream type

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 2: Implement thread retargeting for JAR channels

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8439258 [details] [diff] [review]
Part 2: Implement thread retargeting for JAR channels

I'm not convinced about null-checking mProgressSink and mLoadFlags on the background thread.  I think we should move those checks into FireOnProgress.

r=me with that.

Comment 9

[Jason Duell]

Comment on attachment 8439257 [details] [diff] [review]
Part 1: Attempt to retarget the HTML parser's request regardless of the stream type

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

::: parser/html/nsHtml5StreamParser.cpp
@@ +943,5 @@
> +    rv = threadRetargetableRequest->RetargetDeliveryTo(mThread);
> +  }
> +
> +  if (NS_FAILED(rv)) {
> +    NS_WARNING("Failed to retarget HTML data delivery to the parser thread.");

IIUC this will warn every time we load HTML from a jar:// uri that's backed by a regular jar file instead of an http: link. Not sure how common that is.

Comment 10

[Jason Duell]

Comment on attachment 8439258 [details] [diff] [review]
Part 2: Implement thread retargeting for JAR channels

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

::: modules/libjar/nsJARChannel.cpp
@@ +1035,5 @@
>  
>      // simply report progress here instead of hooking ourselves up as a
>      // nsITransportEventSink implementation.
>      // XXX do the 64-bit stuff for real
> +    if (mProgressSink && NS_SUCCEEDED(rv) && !(mLoadFlags & LOAD_BACKGROUND)) {

mProgressSink only gets initialized in AsyncOpen and nulled out in OnStopRequest, so we're probably fine with the use here.  I also wouldn't expect mLoadFlags to change after AsyncOpen.  So I'm less worried than bz about these.  But I'm fine with moving them to FireOnProgress.

::: modules/libjar/nsJARChannel.h
@@ +15,5 @@
>  #include "nsIRemoteOpenFileListener.h"
>  #include "nsIZipReader.h"
>  #include "nsIDownloader.h"
>  #include "nsILoadGroup.h"
> +#include "nsIThreadRetargetableRequest.h"

Change comment in nsIThreadRetargetableRequest.idl:

   *  ... Should
   * only be called before AsyncOpen for nsIWebsocketChannels, or during
   * OnStartRequest for nsIChannels.
   * Note: For nsIChannels, OnStartRequest and OnStopRequest will still be
   * delivered on the main thread.

i.e. s/nsIHttpChannel/nsIChannel/

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a02ba6359bb
https://hg.mozilla.org/integration/mozilla-inbound/rev/97fa54d04c48

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/9a02ba6359bb
https://hg.mozilla.org/mozilla-central/rev/97fa54d04c48