Closed Bug 1024388 Opened 5 years ago Closed 5 years ago

Allow JAR channels to be retargeted to a different thread.

Categories

(Core :: Networking: JAR, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: khuey, Assigned: khuey)

Details

Attachments

(2 files, 2 obsolete files)

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?
Attachment #8439062 - Flags: feedback?(sworkman)
Attachment #8439062 - Flags: feedback?(jduell.mcbugs)
Attachment #8439062 - Flags: feedback?(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/?
Attachment #8439060 - Flags: review?(bzbarsky) → review+
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.
Attachment #8439062 - Flags: feedback?(bzbarsky) → feedback+
Attachment #8439062 - Flags: feedback?(sworkman)
Attachment #8439062 - Flags: feedback?(jduell.mcbugs)
Attachment #8439062 - Attachment is obsolete: true
Attachment #8439258 - Flags: review?(jduell.mcbugs)
Attachment #8439258 - Flags: review?(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.
Attachment #8439258 - Flags: review?(bzbarsky) → review+
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 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/
Attachment #8439258 - Flags: review?(jduell.mcbugs) → review+
https://hg.mozilla.org/mozilla-central/rev/9a02ba6359bb
https://hg.mozilla.org/mozilla-central/rev/97fa54d04c48
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.