Closed Bug 898524 (NavigationController) Opened 11 years ago Closed 10 years ago

Implement Navigation Controller interfaces for ServiceWorkers

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: ehsan.akhgari, Assigned: jdm)

References

()

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 29 obsolete files)

42.76 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
38.42 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
      No description provided.
Alias: NavigationController
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
Assignee: ehsan → josh
Whiteboard: [games:p?]
Why is this important for games?
Flags: needinfo?(akligman)
I suspect this might be useful for handling managed caching/patching/offline support for games. Just putting it on the radar.
Flags: needinfo?(akligman)
(In reply to comment #2)
> I suspect this might be useful for handling managed caching/patching/offline
> support for games. Just putting it on the radar.

It will hopefully be useful for a better offline story for all kinds of web applications (which of course does include games).  But this is not an issue specific to games in any way.
Whiteboard: [games:p?]
Some details about Blink's implementation... not sure if it's helpful or not:

https://docs.google.com/document/d/1cAumcmAFqcgVbMVPiHmYGJ5ySHbcxm9FynYJoZ1UE-Y/
WIP network integration
Comment on attachment 811323 [details] [diff] [review]
Part 1: Add support for synthesizing http channel responses in place of making a network request.

Jason, if you (or someone on the networking team) could take a look at these changes, I'd be appreciative.
Attachment #811323 - Flags: feedback?(jduell.mcbugs)
Comment on attachment 811325 [details] [diff] [review]
Part 2: Allow synthesizing http channel responses cross-process.

Similarly.
Attachment #811325 - Flags: feedback?(jduell.mcbugs)
jdm, I think this deserves meta-bug status too :)
Summary: Implement NavigationController → [meta] Implement Navigation Controller interfaces for ServiceWorkers
Attachment #811323 - Attachment is obsolete: true
Attachment #811323 - Flags: feedback?(jduell.mcbugs)
Attachment #811325 - Attachment is obsolete: true
Attachment #811325 - Flags: feedback?(jduell.mcbugs)
Comment on attachment 830578 [details] [diff] [review]
Part 1: Add support for synthesizing http channel responses in place of making a network request.

Feedback feedback feedback
Attachment #830578 - Flags: feedback?(jduell.mcbugs)
Comment on attachment 830582 [details] [diff] [review]
Part 2: Allow synthesizing http channel responses cross-process.

Always more feedback.
Attachment #830582 - Flags: feedback?(jduell.mcbugs)
Depends on: 940273
Minor whitespace fixes and modifed OnWriteSegment() to deal with end of stream.
Attachment #830578 - Attachment is obsolete: true
Attachment #830578 - Flags: feedback?(jduell.mcbugs)
Attachment #830582 - Attachment is obsolete: true
Attachment #830582 - Flags: feedback?(jduell.mcbugs)
Attachment #8343443 - Attachment is obsolete: true
Attachment #8343443 - Flags: feedback?(jduell.mcbugs)
Assignee: nsm.nikhil → josh
Status: NEW → ASSIGNED
Attachment #8343445 - Attachment is obsolete: true
Attachment #8343445 - Flags: feedback?(jduell.mcbugs)
Comment on attachment 8395731 [details] [diff] [review]
Part 2: Allow synthesizing http channel responses cross-process.

Time to start getting serious about actually looking at this, Jason :)
Attachment #8395731 - Flags: review?(jduell.mcbugs)
Attachment #8395729 - Flags: review?(jduell.mcbugs)
Rebase to deal with PFileDescriptor.
Attachment #8398773 - Flags: review?(jduell.mcbugs)
Comment on attachment 8395729 [details] [diff] [review]
Part 1: Add support for synthesizing http channel responses in place of making a network request.

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

Approach overall looks like it may work, but there might be alternatives (see comments).  I definitely want Honza or Patrick to take a look at this too.  I didn't look hard at the HttpTransaction stuff yet--they are better reviewers for that than me anyway.

Honza, feel free to fob off review on mcmanus if you're too swamped with cache2 stuff. But since you know appcache I figure you may have some insight here.  Mostly we need to figure out if this is the right general design.

::: netwerk/base/public/nsIAlternateSourceChannel.idl
@@ +8,5 @@
> +interface nsIChannel;
> +interface nsIInputStream;
> +
> +[scriptable, uuid(9bd368b6-3893-4fd7-b7e7-40a9cd45e684)]
> +interface nsIAlternateSourceChannel : nsISupports {

Some sort of comment about what an IDL is for is always a good idea :)

::: netwerk/base/public/nsINetUtil.idl
@@ +193,5 @@
>                                          out AUTF8String aCharset,
>                                          out long aCharsetStart,
>                                          out long aCharsetEnd);
> +
> +  nsIChannel createAlternateSourceChannel(in nsIChannel aChannel);

why not return an nsIAlternateSourceChannel instead of an nsIChannel?

::: netwerk/base/src/AlternateSourceChannel.cpp
@@ +11,5 @@
> +#include "nsSocketTransportService2.h"
> +#include "nsHttpTransaction.h"
> +#include "nsThreadUtils.h"
> +
> +//XXXjdm should probably make QI forward to mWrappedChannel for non-nsIChannel/nsISupports

GetInterface (if/when you need it) should probably forward to mWrappedChannel.  My understanding is that the QI contract guarantees you can QI back to the original IDL, so forwarding QI might not be so easy to do?  Ask someone with more XPCOM chops than me.

@@ +19,5 @@
> +namespace mozilla {
> +namespace net {
> +
> +NS_IMPL_ISUPPORTS3(AlternateSourceChannel,
> +                   nsIChannel,

nsIChannel inherits from nsIRequest, but IIRC that means you still need to include it in this list, no?  See for instance

  http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsBaseChannel.cpp#291

@@ +21,5 @@
> +
> +NS_IMPL_ISUPPORTS3(AlternateSourceChannel,
> +                   nsIChannel,
> +                   nsIAlternateSourceChannel,
> +                   nsIStreamListener)

Ditto--I'd think you need nsIRequestObserver as well as nsIStreamListener.

@@ +31,5 @@
> +, mContentLength(-1)
> +, mForwardToWrapped(false)
> +, mCanceled(false)
> +, mPending(false)
> +, mWasOpened(false)

Indent member variable init list, you barbarian! :)

@@ +67,5 @@
> +}
> +
> +class WriteSegmentsRunnable : public nsRunnable {
> +public:
> +    WriteSegmentsRunnable(nsHttpTransaction* aTransaction,

nit: you switch here to 4-space indents. 2 was better.

@@ +237,5 @@
> +{
> +  NS_ENSURE_TRUE(!mWasOpened, NS_ERROR_FAILURE);
> +
> +  if (mForwardToWrapped) {
> +    return mWrappedChannel->AsyncOpen(aListener, aContext);

This will call AsyncOpen twice on the wrappedChannel, which is wrong.  

From your test code it looks like you want to enforce a calling order of

  AsyncOpen
  {InitiateAltResponse | forwardToOriginalChan}

anything that deviates from that should be detected and failed, unless I'm misunderstanding.

@@ +244,5 @@
> +  mWasOpened = true;
> +
> +  nsCOMPtr<nsIHttpChannelInternal> httpChan = do_QueryInterface(mWrappedChannel);
> +  if (httpChan) {
> +    httpChan->AsyncOpenNetworkless(aListener, aContext);

OK, so you're basically keeping the httpChannel around even if the request is handled by the serviceWorker, just so you can proxy calls to getURI(), etc, down to it.

I haven't looked hard but I'd be curious to see if having AltChannel inherit from HttpBaseChannel would be another way to give you most of what you need here.  Just suggesting it in case there's some hiccup with the current strategy (which seems fine to me so far)

@@ +247,5 @@
> +  if (httpChan) {
> +    httpChan->AsyncOpenNetworkless(aListener, aContext);
> +  } else {
> +    mListener = aListener;
> +    mContext = aContext;

if you don't open the underlying channel, then it will not report IsPending correctly.  You could keep your own state variable here to get that right (true from end of AsyncOpen until OnStopRequest called.)

More generally: what's the case you're expecting where you create an HTTP channel but it doesn't QI to nsIHttpChannelInternal?  I'm not sure there's any way addons/etc are able to prevent you from always getting an nsHttpChannel.  Ask bz.  I thought we took out the capability of JS to override HTTP protocol handler (I certainly don't give a damn about supporting that model).   So I'm not sure you need the 'else' logic.

@@ +329,5 @@
> +
> +NS_IMETHODIMP
> +AlternateSourceChannel::GetLoadGroup(nsILoadGroup** aLoadGroup)
> +{
> +  return mWrappedChannel->GetLoadGroup(aLoadGroup);

I suspect you may want to have AltChannel add itself to the loadGroup too.  Otherwise how are you going to detect/handle the case where the loadGroup gets cancelled?  If you've forwardedToOrigChannel, you'll be fine, but I'm not clear how it'll work for the InitAltResponse case.

@@ +386,5 @@
> +                                      nsresult aStatusCode)
> +{
> +  MOZ_ASSERT(!mForwardToWrapped);
> +
> +  mStatus = aStatusCode;

General convention here is to only overwrite mStatus if it's NS_OK, i.e. keep any canceled status.  see 

  http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#5084

@@ +388,5 @@
> +  MOZ_ASSERT(!mForwardToWrapped);
> +
> +  mStatus = aStatusCode;
> +  if (!mCanceled && NS_FAILED(mStatus)) {
> +    mCanceled = true;

nsHttpChannel reserves setting mCanceled iff Cancel() was called--other errors may set mStatus to a failure code, but they don't set mCanceled.  So I suspect you don't want to do this.

::: netwerk/base/src/nsIOService.cpp
@@ +1178,5 @@
>      return NS_OK;
>  }
>  
> +NS_IMETHODIMP
> +nsIOService::CreateAlternateSourceChannel(nsIChannel* aChannel,

I see you calling this in the test via getService(nsIIOService), but I don't see any change to the nsIIOService IDL.  Am I missing something?

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +218,5 @@
>      , mHasQueryString(0)
>      , mConcurentCacheAccess(0)
>      , mIsPartialRequest(0)
>      , mHasAutoRedirectVetoNotifier(0)
> +    , mDelayTransactionIndefinitely(false)

a bit wordy.  mDelayTransaction or even mDelayed would be better IMO.

@@ +4531,5 @@
> +NS_IMETHODIMP
> +nsHttpChannel::AsyncOpenNetworkless(nsIStreamListener *listener, nsISupports *context)
> +{
> +    mDelayTransactionIndefinitely = true;
> +    return AsyncOpen(listener, context);

So as it stands AsyncOpen is still going to do a few things like

- call on-opening-request and on-modify-request observers.  Do we want that? I'm not clear. It's probably a question for Jonas.  Note that observers can do pretty much anything they want to the channel including redirectTo(URI), etc.  Also note that on-opening-request must happen synchronously within AsyncOpen, so if we're going to call it, we have to call it here (the only client right now I believe is Firebug? But it's a public API)

- add the wrapped channel to the loadGroup if any (and there will be one).  That's probably right (and we prob want to add the AltChannel too as I mention elsewhere)

- we're setting mAsyncOpenTime--that's bad if we delay for long (is that likely, or are we going to generally know if we'll handle via workers or network/cache quickly enough that we won't report artificially long networking load times?)

- we're going to do proxy info lookup (which can involve PAC stuff, etc).

- we'll do DNS prefetch (which is probably fine, unless there's any privacy issue I'm missing with doing DNS when we won't wind up hitting the network)

An alternative strategy here might be to short-circuit the regular AsyncOpen path, and just return if mDelay before we call ResolveProxy/BeginConnect.  We could then add some new function that just called ResolveProxy and AltChannel could use it if we forwardToOriginalChannel.   Looking through the code I'm not finding a lot of stuff that gets set in the ResolveProxy/Connect pathway that needs to be set in order to have a open-looking channel (SetupTransaction sets mRequestHead.SetRequestURI and some cache headers), so I think it might just work.  I'm not sure if it's better though :)  Hopefully Honza and/or Patrick have some thoughts here.

@@ +4541,5 @@
> +    mDelayTransactionIndefinitely = false;
> +    //TODO: open cache entry?
> +    //TODO: speculatively connect?
> +
> +    nsresult rv = gHttpHandler->InitiateTransaction(mTransaction, mPriority);

You definitely can't just init a new transaction--we might have the resource in the cache.

I haven't thought through the logic carefully enough to say if it'll work, but I suspect your best bet is to use some common/existing logic here.  Maybe after setting mDelayTransaction=false you can just call Connect()?  Or in my "stop AsyncOpen before calling ResolveProxy()" approach I mention above, you could just call ResolveProxy(). Honza or Patrick may have thoughts here too.

::: netwerk/protocol/http/nsHttpHeaderArray.h
@@ +14,5 @@
>  class nsIHttpHeaderVisitor;
>  
> +namespace IPC {
> +template<typename T> struct ParamTraits;
> +}

Does this belong in the e10s patch?  Looks like it.
Attachment #8395729 - Flags: review?(jduell.mcbugs)
Attachment #8395729 - Flags: review?(honzab.moz)
Attachment #8395729 - Flags: feedback+
Comment on attachment 8398773 [details] [diff] [review]
Part 2: Allow synthesizing http channel responses cross-process.

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

I've run out of time and will be gone for a week so I'm punting this to honza, too.
Attachment #8398773 - Flags: review?(jduell.mcbugs) → review?(honzab.moz)
This bug has way too many patches to be a [meta] bug any more :)
Summary: [meta] Implement Navigation Controller interfaces for ServiceWorkers → Implement Navigation Controller interfaces for ServiceWorkers
Attachment #8395731 - Attachment is obsolete: true
Attachment #8395731 - Flags: review?(jduell.mcbugs)
Attachment #8398773 - Attachment is obsolete: true
Attachment #8398773 - Flags: review?(honzab.moz)
Don't know the best person to ask for review on this, so asking ehsan since he'll have looked over the ServiceWorkerManager.
Attachment #8400312 - Flags: review?(ehsan)
Ended up deleting a lot of trailing whitespace.
Mainly rebasing.
Attachment #8400321 - Flags: review?(honzab.moz)
Some rebasing.
Dispatches runnable to set non-http channel to 'ready'.
Attachment #8400322 - Flags: review?(honzab.moz)
Attachment #8399664 - Attachment is obsolete: true
Attachment #8399664 - Flags: review?(honzab.moz)
josh, nsm, which of the the Part 1 patch is "the one"?
Flags: needinfo?(nsm.nikhil)
Probably the most recent one, but nsm included one billion unrelated whitespace changes and should probably correct that.
Comment on attachment 8395729 [details] [diff] [review]
Part 1: Add support for synthesizing http channel responses in place of making a network request.

Sorry, bzexport didn't obsolete automatically.
Attachment #8395729 - Attachment is obsolete: true
Attachment #8395729 - Flags: review?(honzab.moz)
Flags: needinfo?(nsm.nikhil)
Attachment #8400321 - Attachment is obsolete: true
Attachment #8400321 - Flags: review?(honzab.moz)
Attachment #8400322 - Attachment is obsolete: true
Attachment #8400322 - Flags: review?(honzab.moz)
FYI - Jonas, is this something your new API proposal may cover?
:jdm, :nsm, didn't catch you on IRC.  As I understand, you want to be able to simulate a full HTTP response in a single byte stream that includes the response line (like HTTP/1.1 200 OK), headers and body.

So you need to pass it to nsHttpTransaction (that does all the parsing) as it would be coming from the net, right?

I can't say I like your implementation approach, but I cannot just that think of anything better my self right now...


According the patches - those are far from review quality: complete lack of comments everywhere, bad indention and formatting on several places.  This cannot get r+ just because of that and makes actual review of the functionality much more complicated.  At least to explain what (and why) you intend the patches to do in a bugzilla comment would be good.

Please give me some time to think of this, currently occupied with cache2.  Feel free to get some feedback from Patrick, maybe he will be able to advice on some nicer approach sooner then me.
If you don't insist on a stream API, could hooking to cache API be an option?  Actually, you would be able to return your own implementation of nsICacheEntry that provides metadata (one of them is the response head) and the content streams.
Comment on attachment 8400312 [details] [diff] [review]
Documents register themselves with ServiceWorkerManager

I've talked to Nikhil offline, this is not ready for review yet.
Attachment #8400312 - Flags: review?(ehsan)
Comment on attachment 8401898 [details] [diff] [review]
Part 1: Add support for synthesizing http channel responses in place of making a network request.

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

This patch is not for a review, but for no more then a feedback

Lack of comments makes the review harder.

The mechanism that pushes the input stream to nsHttpTransaction seems correct.

I didn't get an answer to whether you really need to support the headers+body alternate byte stream or can provide response code/headers/body separately.  In the letter case we could rather hook the cache API and synthesize the response through an always-valid artificial cache entry.

::: netwerk/base/public/nsIAlternateSourceChannel.idl
@@ +8,5 @@
> +interface nsIChannel;
> +interface nsIInputStream;
> +
> +[scriptable, uuid(9bd368b6-3893-4fd7-b7e7-40a9cd45e684)]
> +interface nsIAlternateSourceChannel : nsISupports {

{ on a new line

@@ +12,5 @@
> +interface nsIAlternateSourceChannel : nsISupports {
> +  void forwardToOriginalChannel();
> +  void initiateAlternateResponse(in nsIInputStream body);
> +  readonly attribute nsIChannel wrappedChannel;
> +};

if this is meant for review and not just feedback (actually regardless that!), then here need to be decent comments.  for the whole interface as an overview of how it's used as well

::: netwerk/base/public/nsINetUtil.idl
@@ +193,5 @@
>                                          out AUTF8String aCharset,
>                                          out long aCharsetStart,
>                                          out long aCharsetEnd);
> +
> +  nsIChannel createAlternateSourceChannel(in nsIChannel aChannel);

comments!

::: netwerk/base/src/AlternateSourceChannel.cpp
@@ +67,5 @@
> +}
> +
> +class WriteSegmentsRunnable : public nsRunnable {
> +public:
> +    WriteSegmentsRunnable(nsHttpTransaction* aTransaction,

indent 2 spaces (whole class)

@@ +120,5 @@
> +
> +NS_IMETHODIMP
> +AlternateSourceChannel::InitiateAlternateResponse(nsIInputStream* aBody)
> +{
> +    mBody = aBody;

indent

::: netwerk/base/src/AlternateSourceChannel.h
@@ +46,5 @@
> +  int64_t mContentLength;
> +  bool mForwardToWrapped;
> +  bool mCanceled;
> +  bool mPending;
> +  bool mWasOpened;

use bit fields (if thread safety is not a concern here)
comments

::: netwerk/base/src/nsIOService.cpp
@@ +1184,5 @@
> +{
> +    nsCOMPtr<nsIChannel> wrapper = new AlternateSourceChannel(aChannel);
> +    wrapper.forget(aWrappedChannel);
> +    return NS_OK;
> +}

I'm missing header/idl change for this

::: netwerk/base/src/nsInputStreamPump.h
@@ -59,5 @@
>       * Dispatched (to the main thread) by OnStateStop if it's called off main
>       * thread. Updates mState based on return value of OnStateStop.
>       */
>      nsresult CallOnStateStop();
> -

remove this ws only change
Attachment #8401898 - Flags: review?(honzab.moz)
Comment on attachment 8401901 [details] [diff] [review]
Part 2: Allow synthesizing http channel responses cross-process.

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

add good comments to the code and explanatory comments to bugzilla when submitting a patch

r- for complete lack of commenting

::: netwerk/base/public/nsINetUtil.idl
@@ +13,5 @@
> +[scriptable, function, uuid(1e883bc8-c8e1-4604-98f1-97e5e8e9ef08)]
> +interface nsIAlternateSourceChannelListener : nsISupports
> +{
> +    void onWrappedChannelReady(in nsIAlternateSourceChannel aChannel);
> +};

comments

how this differs from nsINetworklessChannelListener?

::: netwerk/base/src/AlternateSourceChannel.cpp
@@ +150,4 @@
>          nsresult rv = nsInputStreamPump::Create(getter_AddRefs(mPump), aBody);
>          NS_ENSURE_SUCCESS(rv, rv);
>          rv = mPump->AsyncRead(this, nullptr);
>          NS_ENSURE_SUCCESS(rv, rv);

indent

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +217,2 @@
>                                 requestHeaders[i].mValue,
>                                 requestHeaders[i].mMerge);

indent

@@ +549,5 @@
> +HttpChannelParent::RecvInitiateDelayedNetwork()
> +{
> +  nsCOMPtr<nsIAlternateSourceChannel> altSourceChannel = do_QueryInterface(mChannel);
> +  if (altSourceChannel) {
> +    altSourceChannel->ForwardToOriginalChannel();

InitiateDelayedNetwork sounds to me like "open this channel, but wait until we decide whether to go out to the network or synthesize"

but here you forward to the original channel... comments needed

@@ +790,5 @@
>    URIParams uriParams;
>    SerializeURI(newURI, uriParams);
>  
> +  nsCOMPtr<nsIHttpChannel> ihttpChan = do_QueryInterface(mChannel);
> +  nsHttpChannel *httpChan = static_cast<nsHttpChannel *>(ihttpChan.get());

would not mChannel.get() do?

@@ +824,5 @@
>    return NS_OK;
>  }
>  
> +NS_IMETHODIMP
> +HttpChannelParent::OnWrappedChannelReady(nsIAlternateSourceChannel* aChannel)

aChannel is not used, why do you pass it?

::: netwerk/protocol/http/PHttpChannel.ipdl
@@ +81,5 @@
> +  InitiateDelayedNetwork();
> +
> +  // Notify child that no response is forthcoming and one should be synthesized from
> +  // the provided input stream.
> +  SynthesizeResponse(InputStreamParams stream);

some explanation why these two are split is needed

::: netwerk/protocol/http/nsIHttpChannelChild.idl
@@ +16,5 @@
>  
>    // Headers that the channel client has set via SetRequestHeader.
>    readonly attribute RequestHeaderTuples clientSetRequestHeaders;
> +
> +  void synthesizeResponse(in nsIInputStream stream);

comments

::: netwerk/protocol/http/nsIHttpChannelInternal.idl
@@ +40,5 @@
> +[scriptable, function, uuid(1e883bc8-c8e1-4604-98f1-97e5e8e9ef08)]
> +interface nsINetworklessChannelListener : nsISupports
> +{
> +    void onNetworklessChannelReady();
> +};

comments
Attachment #8401901 - Flags: review?(honzab.moz) → review-
(In reply to Honza Bambas (:mayhemer) from comment #35)
> If you don't insist on a stream API, could hooking to cache API be an
> option?  Actually, you would be able to return your own implementation of
> nsICacheEntry that provides metadata (one of them is the response head) and
> the content streams.

Sorry for the really late reply, I was dealing with a lot of plumbing.
I think it is acceptable to have a non-streaming API for the headers. The body itself might end up accepting 'streams' on the JS side at some point in the future. Will that split be enough to allow using the Cache? What would be the easiest implementation approach?
(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #39)
> (In reply to Honza Bambas (:mayhemer) from comment #35)
> > If you don't insist on a stream API, could hooking to cache API be an
> > option?  Actually, you would be able to return your own implementation of
> > nsICacheEntry that provides metadata (one of them is the response head) and
> > the content streams.
> 
> Sorry for the really late reply, I was dealing with a lot of plumbing.
> I think it is acceptable to have a non-streaming API for the headers. The
> body itself might end up accepting 'streams' on the JS side at some point in
> the future. Will that split be enough to allow using the Cache? What would
> be the easiest implementation approach?

That's fantastic!  We can do all of this much simpler and elegant way now.

So, we need to give a channel a prefilled "artificial" cache entry - nsICacheEntry and tell it load from it.  

How to get nsICacheEntry object to work with, there are few ways:
- ask the cache service to create one for you for a specific load context and URI, it may be memory-only or persisted to disk ; it will be tracked and "foundable" in the service hashtables a(and on disk)
- have your own impl of the interface
- we can add a new method on the cache service to create a cache entry for "personal use", such entry would not be tracked by the cache back-end, would not be in any hashtable to be found again and would not persist to disk

In what moment exactly to set an entry on a channel is mostly up to you (must happen before opening the transaction indeed).  

How exactly to set the entry I will explain in detail later - a new interface with ReadFromCacheEntry(nsICacheEntry) would be sufficient.  When the http channel is set the entry and some flags in an appropriate fashion, simple call to ReadFromCache() will start the wheels.

How to prefill the entry: you need to set "response-head" metadata (setMetaDataElement) with a so-called flatten string data of the response line + headers.  See (or even use if you are in C++!) nsHttpResponseHead::Flatten method to get the string value to pass to setMetaDataElement.

To push the data payload, when using the existing new HTTP cache v2 implantation of an entry - just open the output stream on the entry and write to it.  It's a non-blocking fully buffered stream.  You can open it any time, even after you have already given your entry to the channel and that channel started to read data from the entry!
(In reply to Honza Bambas (:mayhemer) from comment #37)
> ::: netwerk/base/src/nsIOService.cpp
> @@ +1184,5 @@
> > +{
> > +    nsCOMPtr<nsIChannel> wrapper = new AlternateSourceChannel(aChannel);
> > +    wrapper.forget(aWrappedChannel);
> > +    return NS_OK;
> > +}
> 
> I'm missing header/idl change for this

This is the nsINetUtil.idl change.
Attachment #8401898 - Attachment is obsolete: true
Assignee: nsm.nikhil → josh
Attachment #8441415 - Flags: review?(honzab.moz)
Attachment #8401901 - Attachment is obsolete: true
(In reply to Honza Bambas (:mayhemer) from comment #38)
> @@ +790,5 @@
> >    URIParams uriParams;
> >    SerializeURI(newURI, uriParams);
> >  
> > +  nsCOMPtr<nsIHttpChannel> ihttpChan = do_QueryInterface(mChannel);
> > +  nsHttpChannel *httpChan = static_cast<nsHttpChannel *>(ihttpChan.get());
> 
> would not mChannel.get() do?

It will not, since mChannel is potentially an AlternateSourceChannel now. The QI is forwarded to the wrapped channel.
I've added a bunch of comments to the interfaces and less clear parts of the code, which I hope is what you were looking for. I've also clean up a couple egregious style problems in both patches.
I think the new suggestion by Honza was to make this interception work by creating custom nsICacheEntry instances and pretending to Necko that our intercepted data had come from the cache.
Attachment #8441353 - Flags: review?(honzab.moz)
Attachment #8441415 - Flags: review?(honzab.moz)
Ah, somehow I neglected to read the more recent discussion.
Comment on attachment 8449498 [details] [diff] [review]
Permit certain HTTP channels to be intercepted before initiating a network connection (WIP)

The test doesn't pass yet (for some reason the cache entry content is ignored), but is this the design that you envision, Honza?
Attachment #8449498 - Flags: feedback?(honzab.moz)
Comment on attachment 8449498 [details] [diff] [review]
Permit certain HTTP channels to be intercepted before initiating a network connection (WIP)

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

can you share some overview of the design?  I will then look at the patch again.

::: netwerk/cache2/nsICacheEntry.idl
@@ +217,5 @@
>     *    false otherwise
>     */
>    boolean hasWriteAccess(in boolean aWriteAllowed);
> +
> +  void asyncOpen(in nsICacheEntryOpenCallback aCallback, in uint32_t flags);

forget about this, this must remain an internal method

please note that you cannot simply have real standalone entries right now, CacheEntry is pretty bound to CacheStorageService pools and hashtables (no plans to change this)

why don't you use the current memory-only storage and give entries an id-extension?

also, we can extend the memory-only storage with an ID argument, so that you can group stuff together globally.

other option would be to have 'standaloneStorage()' on nsICacheStorageService that would contextually group entries, to share entries with same URL, you will carry this storage (each such new storage will have internally it's own ID automatically generated for you)


but I assume this patch is just a very early preview, right?
I'll look into the standaloneStorage you propose; that sounds easiest to work with, from my point of view. The design that I'm going for is that with a properly prepared channel, when we attempt to open a cache entry during the regular connection flow, we sidestep the regular behaviour (ie. fetching a CacheStorage and obtaining a new entry) and open the precreated standalone cache entry instead. This is the first time I've ever looked at the new cache code though, so I welcome any input from you.

I am actually hoping to have a very standalone entry; in the future, it's not going to be acceptable to route data from a service worker in the child process to the cache service in the parent process in order to have it be sent back to the child again, and I'll need to create a usable cache entry in the child. If that's going to be a problem, we're going to have to look for another solution here.
(In reply to Josh Matthews [:jdm] from comment #51)
> I'll look into the standaloneStorage you propose; that sounds easiest to
> work with, from my point of view. The design that I'm going for is that with
> a properly prepared channel, when we attempt to open a cache entry during
> the regular connection flow, we sidestep the regular behaviour (ie. fetching
> a CacheStorage and obtaining a new entry) and open the precreated standalone
> cache entry instead. 

Aha.  We could set the storage you get from (now non-existent) standaloneStorage on the channel, so that the channel will use that customized storage instead of looking into the cache service.  Only question is where to put this attribute or method, on what interface.

The ID of standaloneStorage should be stored in the context key (generated at [1]), I'd give it a tag 'u' as "unique" or "user".

> This is the first time I've ever looked at the new
> cache code though, so I welcome any input from you.
> 
> I am actually hoping to have a very standalone entry; in the future, it's
> not going to be acceptable to route data from a service worker in the child
> process to the cache service in the parent process in order to have it be
> sent back to the child again, and I'll need to create a usable cache entry
> in the child. If that's going to be a problem, we're going to have to look
> for another solution here.

The cache service will well work on child processes too, isolated from the parent process.  It may need some small tweaks, but only very small to make it work as you want, or maybe it already works.

The binding of an entry to the cache service is there since we need to keep the entry somehow alive, but only for a reasonable time - have a memory eviction.  Don't worry that much about it now.


[1] http://hg.mozilla.org/mozilla-central/annotate/b7b20af4a4fb/netwerk/cache2/CacheFileUtils.cpp#l224
Comment on attachment 8449498 [details] [diff] [review]
Permit certain HTTP channels to be intercepted before initiating a network connection (WIP)

I got what I needed via email and #necko.
Attachment #8449498 - Flags: feedback?(honzab.moz)
Still missing documentation, but this passes all the tests that I've thrown at it.
Attachment #8449498 - Attachment is obsolete: true
Attachment #8441415 - Attachment is obsolete: true
Attachment #8441353 - Attachment is obsolete: true
Attachment #8400312 - Attachment is obsolete: true
Attachment #8453254 - Attachment is obsolete: true
Attachment #8453258 - Attachment is obsolete: true
Attachment #8453307 - Attachment is obsolete: true
Comment on attachment 8453310 [details] [diff] [review]
Permit certain HTTP channels to be intercepted before initiating a network connection

Honza, this is nearly ready for review I think, but there are a few questions that I have. I'm currently using the memory cache and appending a 'u' to the extension when intercepting; I don't really know what the implications of this are, but I want to make it so non-intercepted requests will not see these special cache entries. I'm also worried about what happens if there are two intercepted requests to the same URL before the first is complete, since I believe we want to have them be intercepted independently. Finally, I'm not really sure what sort of interface to provide for FinishSynthesizedResponse, since we need to eventually allow modifying all properties of a response (headers, etc.) and modify the new nsHttpResponse object accordingly. Any thoughts?
Attachment #8453310 - Flags: feedback?(honzab.moz)
Comment on attachment 8453310 [details] [diff] [review]
Permit certain HTTP channels to be intercepted before initiating a network connection

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

This is a preliminary feedback, I didn't check the whole patch (e.g. all minor changes in nsHttpChannel and IPC)

Feel free to update the patch tho!


So the flow is:

* channel asyncOpens()
* inside asyncOpen() check for nsINetworkInterceptController and (if not forbidden) asks it whether to intercept, changes the mInterceptCache state
* when interception is engaged, it opens an artificial entry first with "u" extended id
* the artificial entry is immediately set as mSynthesizedCacheEntry
* then opens the same entry the usual way (and waits for the synthetic reference be filled first)
* channel waits for OnCacheEntry* callbacks from the second cache entry open now (is suspended)
* then somebody has to call finishSynthesizedResponse, giving the data
* that opens the output stream on the entry -> onCacheEntryAvail() gets called on the channel
* things moving on

I'd rather expose the output stream to generator of the response, that would save a lot of memory copying.  Content-length is actually not needed to make this work, at least for the run-time generated content (like we have chunked encoding).

To avoid need for InterceptionCacheEntryListener, we may introduce |nsICacheEntry openTruncate(URI, eId);| method on nsICacheStorage.  That would give you a new entry to write to instantly (similar to recreate() method on nsICacheEntry).  I can have a patch in an hour for you.

I also am not 100% happy the channel needs to take care so much (at least keeping the mSynthesizedCacheEntry) about this at all.  Could the interceptor keep the entry and internally join it with the channel?  I would not insist on something like this to give r+, I would just like to avoid similar binding of nsHttpChannel tightly to SW as it now is to AppCache..  There is too much AppCache specific code now :(  So, some middle-man between SW and http channel would be nice.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +64,5 @@
>  #include "nsCRT.h"
>  #include "nsPIDOMWindow.h"
>  #include "nsPerformance.h"
>  #include "CacheObserver.h"
> +#include "CacheStorageService.h"

you don't need this.

@@ +228,5 @@
> +        uint32_t* aFlags)
> +{
> +    // This should never be called, as the listener should always be the first
> +    // consumer of the synthesized cache entries.
> +    return NS_ERROR_NOT_IMPLEMENTED;

if the entry already exists, this will make OnCacheEntryAvailable get NS_ERROR_CACHE_ETNRY_NOT_FOUND with null aEntry

only in case you OPEN_TRUNCATE, this will never be called for the listener.

@@ +2552,5 @@
>  
>      // make sure we're not abusing this function
>      NS_PRECONDITION(!mCacheEntry, "cache entry already open");
>  
>      nsAutoCString cacheKey;

is cacheKey actually used?

@@ +2661,5 @@
> +    // readable cache entry, to avoid causing a network request to occur in order to
> +    // fill it up.
> +    if (mInterceptCache != DO_NOT_INTERCEPT) {
> +        nsCOMPtr<nsICacheEntryOpenCallback> callback = new InterceptionCacheEntryListener(this);
> +        rv = cacheStorage->AsyncOpenURI(openURI, extension, cacheEntryOpenFlags, callback);

cacheStorage may well be a disk storage here...

@@ +3704,5 @@
>              doom = true;
>      }
>      else if (mCacheEntryIsWriteOnly)
>          doom = true;
> +    else if (mInterceptCache == INTERCEPTED) // ensure the cache entry is only used once

these extra dooms would be solved with the standaloneStorage(), right?  I planned it would behave the way that when released (the storage) all would go away automatically.

@@ +5011,5 @@
> +    rv = mSynthesizedCacheEntry->OpenOutputStream(0, getter_AddRefs(out));
> +    NS_ENSURE_SUCCESS_VOID(rv);
> +
> +    nsCOMPtr<nsIEventTarget> target;
> +    rv = CacheStorageService::Self()->GetIoTarget(getter_AddRefs(target));

you have xpcom api for that.

@@ +5013,5 @@
> +
> +    nsCOMPtr<nsIEventTarget> target;
> +    rv = CacheStorageService::Self()->GetIoTarget(getter_AddRefs(target));
> +    NS_ENSURE_SUCCESS_VOID(rv);
> +    rv = NS_AsyncCopy(mSynthesizedResponse, out, target);

nusty memcopy!  yuk!

@@ +5020,5 @@
> +    mSynthesizedCacheEntry = nullptr;
> +}
> +
> +NS_IMETHODIMP
> +nsHttpChannel::FinishSynthesizedResponse(nsIInputStream* aStream)

I remember you were asking on IRC how to generate the response asynchronously... so why are you providing an input stream to read the response from then rather give the response generator the output stream of the synthetic entry?  that was what I was suggesting.

::: netwerk/protocol/http/nsIHttpChannelInternal.idl
@@ +212,5 @@
> +     * Instruct a channel that has been intercepted to use the provided input stream
> +     * as a synthesized response in place of making a network request.
> +     *
> +     * XXXjdm This needs more parameters to control the headers of the synthesized
> +     *        response.

It would better to just give the generator the whole entry (or some reduced wrapper interface) to fill it.
Comment on attachment 8453310 [details] [diff] [review]
Permit certain HTTP channels to be intercepted before initiating a network connection

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

f- only for the IPC bits, I think we can do it better.

::: netwerk/base/public/nsINetworkInterceptController.idl
@@ +14,5 @@
> + * request should be intercepted before any network request is initiated.
> + */
> +
> +[scriptable, uuid(b3ad3e9b-91d8-44d0-a0c5-dc2e9374f599)]
> +interface nsINetworkInterceptController : nsISupports

can I see the implementation somewhere?

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +579,5 @@
> +  nsTArray<mozilla::ipc::FileDescriptor> fds;
> +  nsCOMPtr<nsIInputStream> stream = DeserializeInputStream(synthesized, fds);
> +  MOZ_ASSERT(fds.IsEmpty());
> +
> +  nsresult rv = mChannel->FinishSynthesizedResponse(stream);

seems like SW will always run on the child process.  hence I think to avoid this horrible ping pong between processes that will enormously eat memory rather do the synthetization on the child process only - in HttpChannelChild.  We may need some more tools for that, or maybe just a simple pipe or storage stream overlapped with input pump will do?
Attachment #8453310 - Flags: feedback?(honzab.moz) → feedback-
(In reply to Honza Bambas (:mayhemer) from comment #59)
> @@ +5020,5 @@
> > +    mSynthesizedCacheEntry = nullptr;
> > +}
> > +
> > +NS_IMETHODIMP
> > +nsHttpChannel::FinishSynthesizedResponse(nsIInputStream* aStream)
> 
> I remember you were asking on IRC how to generate the response
> asynchronously... so why are you providing an input stream to read the
> response from then rather give the response generator the output stream of
> the synthetic entry?  that was what I was suggesting.

Obtaining an output stream requires calling OpenOutputStream, which implicitly marks the entry as ready. If we haven't provided the response-head metadata yet, the entry is not usable and the channel ignores it when checking if it's valid. I don't see any way to bridge the divide between the response creator and the channel, because the response head needs to include Content-Length - at some point, the full response needs to be ready before we can create an output stream, so there's not much point in creating an API that hands off the output stream to some other location, since the response must already be present.
(In reply to Honza Bambas (:mayhemer) from comment #60)
> Comment on attachment 8453310 [details] [diff] [review]
> Permit certain HTTP channels to be intercepted before initiating a network
> connection
> 
> Review of attachment 8453310 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> f- only for the IPC bits, I think we can do it better.
> 
> ::: netwerk/base/public/nsINetworkInterceptController.idl
> @@ +14,5 @@
> > + * request should be intercepted before any network request is initiated.
> > + */
> > +
> > +[scriptable, uuid(b3ad3e9b-91d8-44d0-a0c5-dc2e9374f599)]
> > +interface nsINetworkInterceptController : nsISupports
> 
> can I see the implementation somewhere?

The only user is in the included tests so far. It doesn't seem worth it to make nsDocument inherit from the new interface and make it always return false.

> ::: netwerk/protocol/http/HttpChannelParent.cpp
> @@ +579,5 @@
> > +  nsTArray<mozilla::ipc::FileDescriptor> fds;
> > +  nsCOMPtr<nsIInputStream> stream = DeserializeInputStream(synthesized, fds);
> > +  MOZ_ASSERT(fds.IsEmpty());
> > +
> > +  nsresult rv = mChannel->FinishSynthesizedResponse(stream);
> 
> seems like SW will always run on the child process.  hence I think to avoid
> this horrible ping pong between processes that will enormously eat memory
> rather do the synthetization on the child process only - in
> HttpChannelChild.  We may need some more tools for that, or maybe just a
> simple pipe or storage stream overlapped with input pump will do?

While this is true, and the plan is to move this all into the child process, I really want to do that work separately. This is a stopgap measure to demonstrate it working in IPC mode.
Honza, what are you thoughts on comment 61? I'm finding it difficult to figure out how to move forward here, since the API designs (providing a direct nsIOutputStream vs. taking an nsIInputStream) are incompatible.
Flags: needinfo?(honzab.moz)
This took less work than I expected. It feels like it could be fragile; transparently setting up another channel and proxying the calls to the original one seems easy to get wrong in various edge cases.
Attachment #8466415 - Flags: feedback?(jduell.mcbugs)
Attachment #8466415 - Flags: feedback?(honzab.moz)
Patrick told me that my comments about the stream API seemed fine, so no need to address comment 61 right now.
Flags: needinfo?(honzab.moz)
Comment on attachment 8466415 [details] [diff] [review]
Part 2 - Avoid IPC roundtrips for synthesized responses

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

It generally looks good.  I'm just worried a bit about using the nsHttpChannel on a child process.  Not sure what all issues it could bring.

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +1762,5 @@
>  HttpBaseChannel::ShouldIntercept()
>  {
>    nsCOMPtr<nsINetworkInterceptController> controller;
>    GetCallback(controller);
> +  bool shouldIntercept = mInterceptForced ? mForceIntercept : false;

|mInterceptForced && mForceIntercept|  ?

::: netwerk/protocol/http/HttpBaseChannel.h
@@ +385,5 @@
>    nsCOMPtr<nsIPrincipal>            mPrincipal;
>  
>    bool                              mForcePending;
> +  bool                              mInterceptForced;
> +  bool                              mForceIntercept;

could these be bit fields?

the names are misleading.

s/mInterceptForced/mInterceptFlagForced/ ?
s/mForceIntercept/mInterceptionFlag/ ?

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +1191,5 @@
>    return NS_OK;
>  }
>  
> +class InterceptStreamListener : public nsIStreamListener
> +                              , public nsIProgressEventSink

comment what this is, how is it used, lifetime, all facts, even when you only want feedback please.

@@ +1327,5 @@
> +    chan->SetLoadFlags(mLoadFlags |
> +                      nsHttpChannel::LOAD_ONLY_FROM_CACHE |
> +                      nsHttpChannel::LOAD_NO_NETWORK_IO);
> +    chan->SetAllowSTS(false);
> +    chan->ForceIntercept(true);

when you set this on nsHttpChannel, what exactly is it doing?  there is the whole synthetic cache entry play?  I think the cache service works on the child process, just stores only in memory.
Attachment #8466415 - Flags: feedback?(honzab.moz) → feedback+
Just rebased; so substantive feedback changes applied.
Attachment #8453310 - Attachment is obsolete: true
Just rebased; so substantive feedback changes applied.
Attachment #8486397 - Attachment description: Permit certain HTTP channels to be intercepted before initiating a network connection → Part 1 - Permit certain HTTP channels to be intercepted before initiating a network connection
Attachment #8466415 - Attachment is obsolete: true
Attachment #8466415 - Flags: feedback?(jduell.mcbugs)
Blocks: 1065216
This addresses all of your feedback except the standaloneStorage one, because the concept is less clear to me.
Attachment #8495401 - Flags: review?(honzab.moz)
Attachment #8486397 - Attachment is obsolete: true
This addresses your feedback on the second patch; the changes mostly relate to the API redesign from the first patch.
Attachment #8495403 - Flags: review?(honzab.moz)
Attachment #8486398 - Attachment is obsolete: true
Here's the new design, which better separates the implementation of synthesizing responses from the implementation of nsHttpChannel:

* something (such as nsDocument) implements nsINetworkInterceptionController
* a new channel being opened passes its URI to nsINetworkInterceptionController.shouldPrepareForIntercept; if it returns false, the request proceeds as normal
* otherwise, the channel continues to execute the regular AsyncOpen flow, but in OpenCacheEntry it calls OpenTruncate on a memory-only cache storage, passes the resulting cache entry to an InterceptedChannel object, then returns without calling AsyncOpenURI for its own cache entry
* the InterceptedChannel object stores the writable cache entry handle and notifies the nsINetworkInterceptionController, passing the output stream for the cache entry body
* if InterceptedChannel::ResetInterception is called, the InterceptedChannel object throws away the writeable cache entry and forces the original channel to redirect to the same URI, avoiding any interception this time
* if InterceptedChannel::FinishSynthesizedResponse is called, the synthesized response body is flattened and stored in the cache entry metadata, the writeable cache entry handle is thrown away, and the original channel calls OpenCacheEntry again to force the creation of a read-only cache entry handle for the synthesized content

The changes to CacheEntry are necessary because the call to AsyncOpenURI in OpenCacheEntry causes the cache callback to be called synchronously (despite the name), and the code that decides whether to proceed with the connection or wait for more callbacks becomes confused. By forcing an asynchronous OnCacheEntryAvailable call this problem is avoided.
Comment on attachment 8495401 [details] [diff] [review]
Part 1: Permit certain HTTP channels to be intercepted before initiating a network connection

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

The cache entry changes are unacceptable (huge perf impact, I'll never r+ it).  If you really need it, we can resurrect bug 938186.  Only problem with patch in that bug was that the test didn't behave on m-i (but did on try!).
(In reply to Honza Bambas (:mayhemer) from comment #72)
> Comment on attachment 8495401 [details] [diff] [review]
> Part 1: Permit certain HTTP channels to be intercepted before initiating a
> network connection
> 
> Review of attachment 8495401 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The cache entry changes are unacceptable (huge perf impact, I'll never r+
> it).  If you really need it, we can resurrect bug 938186.  Only problem with
> patch in that bug was that the test didn't behave on m-i (but did on try!).

Bug 938186 does allow me to revert the cache changes in this patch, and the patch doesn't work without it.
Comment on attachment 8495401 [details] [diff] [review]
Part 1: Permit certain HTTP channels to be intercepted before initiating a network connection

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

::: netwerk/base/public/nsINetworkInterceptController.idl
@@ +48,5 @@
> +interface nsINetworkInterceptController : nsISupports
> +{
> +    /**
> +     * Returns true if a channel should avoid initiating any network
> +     * requests until specifically instructed to do so.

needs docs on the args

@@ +54,5 @@
> +    bool shouldPrepareForIntercept(in nsIURI aURI);
> +
> +    /**
> +     * Notification when a given intercepted channel is prepared to accept a synthesized
> +     * repsonse via the provided stream.

needs docs on the args

::: netwerk/cache2/CacheEntry.h
@@ +220,5 @@
>    void InvokeCallbacksLock();
>    void InvokeCallbacks();
>    bool InvokeCallbacks(bool aReadOnly);
>    bool InvokeCallback(Callback & aCallback);
> +  void InvokeAvailableCallback(Callback const & aCallback, bool forceSync=true);

as talked about it, this will handle bug 938186

::: netwerk/protocol/http/HttpBaseChannel.h
@@ +390,5 @@
>  
>    nsCOMPtr<nsIPrincipal>            mPrincipal;
>  
>    bool                              mForcePending;
> +  bool                              mForceNoIntercept;

maybe put this amid the other bit fields?

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +64,5 @@
>  #include "nsCRT.h"
>  #include "nsPIDOMWindow.h"
>  #include "nsPerformance.h"
>  #include "CacheObserver.h"
> +#include "CacheStorageService.h"

not happy to see this.... why is this actually needed?

@@ +202,5 @@
>  }
>  
> +// An object representing a channel that has been intercepted. This avoids complicating
> +// the actual channel implementation with the details of synthesizing responses.
> +class InterceptedChannel : public nsIInterceptedChannel {

class XX
{
};

@@ +209,5 @@
> +
> +    // The interception controller to notify about the successful channel interception
> +    nsCOMPtr<nsINetworkInterceptController> mController;
> +
> +    // Original intercepted URL

and why do you keep it?

@@ +229,5 @@
> +    {
> +        mChannel->GetURI(getter_AddRefs(mURI));
> +    }
> +
> +    void Notify(nsICacheEntry* aEntry);

what does this do?

@@ +2667,5 @@
> +        NS_ENSURE_SUCCESS(rv, rv);
> +        nsCOMPtr<nsINetworkInterceptController> controller;
> +        GetCallback(controller);
> +        nsRefPtr<InterceptedChannel> intercepted = new InterceptedChannel(this, controller);
> +        intercepted->Notify(entry);

what does the notification do here?

please add some blank lines between the logical blocks to make this more readable

@@ +6347,5 @@
> +    }
> +
> +    EnsureSynthesizedResponse();
> +    nsAutoCString header = aName + NS_LITERAL_CSTRING(": ") + aValue;
> +    nsresult rv = mSynthesizedResponseHead->ParseHeaderLine(header.get());

just a note this will overwrite any previous headers set.

@@ +6373,5 @@
> +    rv = DoAddCacheEntryHeaders(mChannel, mSynthesizedCacheEntry, mChannel->GetRequestHead(),
> +                                mSynthesizedResponseHead.ptr(), securityInfo);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    // Ensure the write lock is released

hmm... "write lock" (actually entry access lock) is released when you open the output stream (one of the ways to release it).  since that moment the entry is available to others immediately to read metadata and open input stream and pump-read it (will be waiting for the output to close/be dropped tho).

So I'm not sure how your "write locking" - as you call it - is intended to work.

Also, to make 100% sure the entry doesn't go away from memory, you should release it AFTER you call mChannel->OpenCacheEntry.

::: netwerk/protocol/http/nsHttpChannel.h
@@ +139,5 @@
>      }
>  
> +    nsresult OpenCacheEntry(bool usingSSL);
> +
> +    nsresult StartRedirectChannelToURI(nsIURI *, uint32_t);

why are you moving this?  If not necessary, please don't do that.
Attachment #8495401 - Flags: review?(honzab.moz) → review-
(In reply to Josh Matthews [:jdm] from comment #71)
> The changes to CacheEntry are necessary because the call to AsyncOpenURI in
> OpenCacheEntry causes the cache callback to be called synchronously (despite
> the name), and the code that decides whether to proceed with the connection
> or wait for more callbacks

Which code you mean?  nsHttpChannel code after your modifications or what?  What is the exact problem?  Maybe we can solve it outside the cache.

>  becomes confused. By forcing an asynchronous
> OnCacheEntryAvailable call this problem is avoided.
The problem is the AutoCacheWaitFlags in OpenCacheEntry that toggles the flags in mCacheEntriesToWaitFor; when we call OnCacheEntryAvailable synchronously, the check in OnCacheEntryAvailableInternal sees that mCacheEntriesToWaitFor is still non-zero (since the AutoCacheWaitFlags is still on the stack) and does not call ContinueConnect.
(In reply to Josh Matthews [:jdm] from comment #76)
> The problem is the AutoCacheWaitFlags in OpenCacheEntry that toggles the
> flags in mCacheEntriesToWaitFor; when we call OnCacheEntryAvailable
> synchronously, the check in OnCacheEntryAvailableInternal sees that
> mCacheEntriesToWaitFor is still non-zero (since the AutoCacheWaitFlags is
> still on the stack) and does not call ContinueConnect.

That more sounds like a bug to me that we don't go on after cache opening is done...  I'll check on that.
(In reply to Honza Bambas (:mayhemer) from comment #77)
> (In reply to Josh Matthews [:jdm] from comment #76)
> > The problem is the AutoCacheWaitFlags in OpenCacheEntry that toggles the
> > flags in mCacheEntriesToWaitFor; when we call OnCacheEntryAvailable
> > synchronously, the check in OnCacheEntryAvailableInternal sees that
> > mCacheEntriesToWaitFor is still non-zero (since the AutoCacheWaitFlags is
> > still on the stack) and does not call ContinueConnect.
> 
> That more sounds like a bug to me that we don't go on after cache opening is
> done...  I'll check on that.

Got it.  Not a bug!  The thing is that when you OpenCacheEntry from nsHttpChannel::Connect, the channel will end up by a call to ContinueConnect().  All you have to do is mimic this behavior in your code - when mCacheEntriesToWaitFor == 0 after you return from OpenCacheEntry, call ContinueConnect() on the channel.
Ok, thanks!
Comment on attachment 8495403 [details] [diff] [review]
Part 2 - Avoid IPC roundtrips for synthesized responses

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

::: netwerk/protocol/http/HttpBaseChannel.h
@@ +391,5 @@
>    nsCOMPtr<nsIPrincipal>            mPrincipal;
>  
>    bool                              mForcePending;
> +  bool                              mInterceptFlagForced : 1;
> +  bool                              mInterceptionFlag : 1;

documentation still needed

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +1236,5 @@
> +  nsRefPtr<HttpChannelChild> mOwner;
> +  bool mPassthrough;
> +  virtual ~InterceptStreamListener() {}
> +public:
> +  InterceptStreamListener(HttpChannelChild* aOwner) : mOwner(aOwner), mPassthrough(true) {}

format this please

@@ +1366,5 @@
> +    chan->SetNotificationCallbacks(mCallbacks);
> +    // no need to make any network request, so let's be strict about that.
> +    chan->SetLoadFlags(mLoadFlags |
> +                      nsHttpChannel::LOAD_ONLY_FROM_CACHE |
> +                      nsHttpChannel::LOAD_NO_NETWORK_IO);

1) there might be bugs on LOAD_NO_NETWORK_IO that makes the channel still try to reach the network (don't recall any #s now).  

2) I believe it's a big foot gun to instantiate nsHttpChannel on the child side.

3) using the cache service on the child is also not a really good idea until absolutely necessary


I've suggested to use just a stream and a response head to do this.  Why do you actually need the whole very expensive nsHttpChannel here?

Why don't you use a transport or a pump over a pipe or a storage stream?  That will give you OnStart/OnData/OnStop as well, progress can be easily synthesized too.

The interceptor you call on via callbacks gets only the output stream and not the whole cache entry, hence you should be OK.

::: netwerk/protocol/http/nsIHttpChannelInternal.idl
@@ +203,5 @@
>      readonly attribute PRTime lastModifiedTime;
>  
>      /**
>       * Force a channel that has not been AsyncOpen'ed to skip any check for possible
> +     * interception and immediately prepare for a possibly synthesized response.

please recheck this comment (doesn't make much sense to me) and document the argument
Attachment #8495403 - Flags: review?(honzab.moz) → review-
(In reply to Honza Bambas (:mayhemer) from comment #74)
> Comment on attachment 8495401 [details] [diff] [review]
> Part 1: Permit certain HTTP channels to be intercepted before initiating a
> network connection
> 
> Review of attachment 8495401 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: netwerk/protocol/http/nsHttpChannel.h
> @@ +139,5 @@
> >      }
> >  
> > +    nsresult OpenCacheEntry(bool usingSSL);
> > +
> > +    nsresult StartRedirectChannelToURI(nsIURI *, uint32_t);
> 
> why are you moving this?  If not necessary, please don't do that.

It's to allow InterceptedChannel to call the private methods. The alternative is to make InterceptedChannel a friend.
Attachment #8495403 - Attachment is obsolete: true
Now with no nsHttpChannel or cache service use. The random changes in http/ files are required for InterceptedChannel.cpp to build.
Attachment #8501749 - Attachment is obsolete: true
Attachment #8495401 - Attachment is obsolete: true
Comment on attachment 8501750 [details] [diff] [review]
Part 1: Permit certain HTTP channels to be intercepted before initiating a network connection

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

r=honzab with comments fixed

::: netwerk/base/public/nsINetworkInterceptController.idl
@@ +25,5 @@
> +     */
> +    void resetInterception();
> +
> +    /**
> +     * Attach a header name/value pair to the forthcoming synthesized response.

Maybe note that this will overwrite any previously set headers.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +6417,5 @@
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    mSynthesizedCacheEntry = nullptr;
> +
> +    rv = mChannel->ContinueConnect();

you must do this only when mCacheEntriesToWaitFor == 0!
Attachment #8501750 - Flags: review?(honzab.moz) → review+
Comment on attachment 8501748 [details] [diff] [review]
Part 2 - Avoid IPC roundtrips for synthesized responses

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

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +611,5 @@
>      // (although we really shouldn't receive any msgs after OnStop),
>      // so make sure this goes out of scope before then.
>      AutoEventEnqueuer ensureSerialDispatch(mEventQ);
>  
> +    DoOnStopRequest(this, mListenerContext, mStatus);

maybe set mIsPending to false ALSO before entering the autoenquer?  and the status as well.  timing on those two is critical

@@ +1895,5 @@
> +  mInterceptListener = nullptr;
> +
> +  // Continue with the original cross-process request
> +  nsresult rv = ContinueAsyncOpen();
> +  NS_ENSURE_SUCCESS_VOID(rv);

maybe you need to asyncAbort (or just abort) on a failure.

@@ +1908,5 @@
> +
> +  // if this channel has been suspended previously, the pump needs to be
> +  // correspondingly suspended now that it exists.
> +  for (uint32_t i = 0; i < mSuspendCount; i++) {
> +    mSynthesizedResponsePump->Suspend();

I'm not sure, but there may be a need to call Suspend after AsyncRead.  Just check on it before landing.

::: netwerk/protocol/http/InterceptedChannel.cpp
@@ +159,5 @@
> +
> +void
> +InterceptedChannelContent::NotifyController()
> +{
> +  nsresult rv = NS_NewStorageStream(4096, UINT32_MAX, getter_AddRefs(mSynthesizedStorage));

just a note that storage stream holds the data until it's threw away.  depends on how many parallel (or sequential) consumers of the stream can be here.  tho, it seems like there is always just the single channel reading it, so a pipe might be better, since the data written to it are threw away immediately after being read.  up to you.

::: netwerk/protocol/http/InterceptedChannel.h
@@ +54,5 @@
> +
> +class InterceptedChannelContent : public nsIInterceptedChannel
> +{
> +  // The actual channel being intercepted.
> +  nsRefPtr<HttpChannelChild> mChannel;

an InterceptedChannelBase class might be nice, up to you.
Attachment #8501748 - Flags: review?(honzab.moz) → review+
https://hg.mozilla.org/mozilla-central/rev/35824fa21176
https://hg.mozilla.org/mozilla-central/rev/b02c0d6ff380
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
No longer depends on: 940273
The service workers API has all been documented (and needs a tech review.)

You can find the controller-related stuff in there, for example https://developer.mozilla.org/en-US/docs/Web/API/ServiceWorkerContainer
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.