1024730 - implement nsIHttpPushListener for associated channels to accept h2 pushes

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement)

Description

[Ben Brittain (:bbrittain)]

Implement simplepush using HTTP/2 Push as described in http://tools.ietf.org/html/draft-thomson-webpush-http2-00

Comment 1

[Ben Brittain (:bbrittain)]

Attachment: C++/idl Push Components

Pretty basic stuff. I'm a little worried about where I have the runnable located, but it certainly works :P

Comment 2

[Ben Brittain (:bbrittain)]

Attachment: 1024730-http2

left in a commented out line :/

Comment 3

[u408661]

Comment on attachment 8455720 [details] [diff] [review]
1024730-http2

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

It would be good to see an example of an nsIPushListener implementation and interaction with this code, to see how it all fits together.

::: netwerk/protocol/http/Http2Push.cpp
@@ +18,5 @@
>  #include "Http2Push.h"
> +#include "nsHttpConnectionMgr.h"
> +#include "nsHttpTransaction.h"
> +#include "nsHttpChannel.h"
> +#include "nsHttpConnection.h"

Why did you add conn mgr and connection includes here? They don't appear to be used at all.

@@ +35,5 @@
> +      , mHost(host)
> +  {
> +  }
> +
> +    NS_IMETHOD

Bad indentation all over this class.

@@ +44,5 @@
> +        if (mPath == EmptyCString() || mPath == NullCString()){
> +          return NS_ERROR_FAILURE;
> +        }
> +        if (mHost == EmptyCString() || mHost == NullCString()){
> +          return NS_ERROR_FAILURE;

Do these checks before sending this event to the main thread. No need to run the event if it's not going to do anything.

@@ +50,5 @@
> +
> +        nsCOMPtr<nsIURI> uri;
> +        nsresult rv;
> +        rv = mTrans->HttpChannel()->GetURI(getter_AddRefs(uri));
> +        nsHttpChannel * chan = static_cast<nsHttpChannel *>(mTrans->HttpChannel());

Just call mTrans->HttpChannel() once, and store it for both these uses.

@@ +99,5 @@
>  nsresult
>  Http2PushedStream::WriteSegments(nsAHttpSegmentWriter *writer,
>                                   uint32_t count, uint32_t *countWritten)
>  {
> +  LOG3(("Http2PushedStream::WriteSegments ctor this=%p", this));

This is not a ctor

@@ +116,5 @@
> +
> +  nsHttpTransaction *trans = mAHttpTransaction->QueryHttpTransaction();
> +  if (trans) {
> +    nsCString path = this->Path();
> +    nsCString host = this->Host();

You don't need to use "this->" here.

@@ +117,5 @@
> +  nsHttpTransaction *trans = mAHttpTransaction->QueryHttpTransaction();
> +  if (trans) {
> +    nsCString path = this->Path();
> +    nsCString host = this->Host();
> +    NS_DispatchToMainThread(new PushListener(trans, host, path));

So WriteSegments may, in fact, be called more than once per stream. You likely don't want to be sending a push notification for the second (and later) times you receive data for a particular URI. You probably want to keep a flag that says whether or not you've actually sent the push notification already.

::: netwerk/protocol/http/Http2Session.cpp
@@ +22,5 @@
>  #include "mozilla/Telemetry.h"
>  #include "mozilla/Preferences.h"
>  #include "nsHttp.h"
>  #include "nsHttpHandler.h"
> +#include "nsHttpChannel.h"

This does not appear to be used anywhere.

@@ +29,5 @@
>  #include "nsISSLSocketControl.h"
>  #include "nsISSLStatus.h"
>  #include "nsISSLStatusProvider.h"
>  #include "nsISupportsPriority.h"
> +#include "nsIChannel.h"

This does not appear to be used anywhere.

::: netwerk/protocol/http/Http2Session.h
@@ +14,5 @@
>  #include "nsClassHashtable.h"
>  #include "nsDataHashtable.h"
>  #include "nsDeque.h"
>  #include "nsHashKeys.h"
> +#include "nsHttpChannel.h"

This does not appear to be used anywhere

@@ +283,5 @@
>    // from the first transaction on this session. That object contains the
>    // pointer to the real network-level nsHttpConnection object.
>    nsRefPtr<nsAHttpConnection> mConnection;
>  
> +  nsCOMPtr<nsIHttpChannelInternal> mHttpChannel;

This does not appear to be used anywhere.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +246,5 @@
> +nsHttpChannel::Push(nsCString host, nsCString path)
> +{
> +    MOZ_ASSERT(NS_IsMainThread());
> +
> +    nsCOMPtr<nsIPushListener> push;

Call this pushListener

@@ +260,5 @@
> +
> +    return NS_OK;
> +}
> +
> +    nsresult

Bad indentation.

::: netwerk/protocol/http/nsHttpChannel.h
@@ +96,5 @@
>      virtual nsresult Init(nsIURI *aURI, uint32_t aCaps, nsProxyInfo *aProxyInfo,
>                            uint32_t aProxyResolveFlags,
>                            nsIURI *aProxyURI);
>  
> +    nsresult Push(nsCString host, nsCString path);

This would be better named as OnPush or OnPushReceived or something like that. Naming it this way makes it sound like you're expecting the channel to do the pushing.

::: netwerk/protocol/http/nsHttpTransaction.h
@@ +195,5 @@
>      nsCOMPtr<nsIHttpActivityObserver> mActivityDistributor;
>  
>      nsCString                       mReqHeaderBuf;    // flattened request headers
>      nsCOMPtr<nsIInputStream>        mRequestStream;
> +    nsCOMPtr<nsIChannel>            mHttpChannel;

If you're assuming this is an HttpChannel, you should store it as an nsCOMPtr<nsIHttpChannel> or nsCOMPtr<nsIHttpChannelInternal>, because otherwise you run the risk of some future changes messing up your assumption.

Comment 4

[Patrick McManus [:mcmanus]]

Comment on attachment 8455720 [details] [diff] [review]
1024730-http2

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

ben - cool!

Since hurley already r-'d it, I only looked at the idl interface bits this time around. I'll review the implementation next time.

::: netwerk/base/public/nsIPushListener.idl
@@ +14,5 @@
> + *
> + */
> +
> +[scriptable, uuid(e63093c5-7fab-4a25-972a-8ad7c5c82b4a)]
> +interface nsIPushListener : nsIStreamListener

let's call this an nsIHttpPushListener..

please define that if the push is accepted the listener will receive onstart, 0..N ondata's, and one onstop just as if a successful AsyncOpen had happened

also define (and implement) that the initial loadgroup, security callbacks, owner and loadinfo are inherited from the associated request (if any)

@@ +19,5 @@
> +{
> +    /**
> +     * Trigger on push
> +     *
> +     * @param host

we need a lot more than host and path.. the on push recipient should be able to see the whole pushed request. I think this arg should be an nsIHttpChannel. That way the call-eee can get a full origin from the URI and see the pushed request headers, etc..

It should probably also include the associated channel as a separate arg. (might be null if pushed on stream 0)

@@ +24,5 @@
> +     *        the host that pushed the resource
> +     * @param path
> +     *        the path of the resource which was pushed
> +     */
> +    void onPush(in ACString host,

probly not void :)

the recipient of onPush() should be able to decline the push, probably by the return code of the method.

Comment 5

[Ben Brittain (:bbrittain)]

Attachment: 1024730-http2

Comment 6

[u408661]

Comment on attachment 8465122 [details] [diff] [review]
1024730-http2

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

Drive-by feedback!

::: netwerk/protocol/http/Http2Push.cpp
@@ +15,5 @@
>  
>  #include <algorithm>
>  
>  #include "Http2Push.h"
> +#include "nsHttpConnectionMgr.h"

This appears to be unused, take it out.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +267,5 @@
> +        NS_NewChannel(getter_AddRefs(pushChannel), pushResource);
> +
> +        // Call the idl code
> +        pushListener->OnPush(this,
> +                             static_cast<nsIHttpChannel*>(pushChannel.get()),

You should really QI this, instead of doing a static cast.

::: netwerk/protocol/http/nsHttpChannel.h
@@ +21,5 @@
>  #include "nsIThreadRetargetableStreamListener.h"
>  #include "nsWeakReference.h"
>  #include "TimingStruct.h"
>  #include "AutoClose.h"
> +#include "Http2Push.h"

This is unused, and duplicated in the .cpp. Kill this one.

@@ +30,5 @@
>  class nsICancelable;
>  class nsIHttpChannelAuthProvider;
>  class nsInputStreamPump;
>  class nsPerformance;
> +class Http2PushedStream;

This forward declaration does not seem to be used.

@@ +97,5 @@
>      virtual nsresult Init(nsIURI *aURI, uint32_t aCaps, nsProxyInfo *aProxyInfo,
>                            uint32_t aProxyResolveFlags,
>                            nsIURI *aProxyURI);
>  
> +    nsresult onPush(nsCString uri);

Capitalize this - OnPush

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +13,5 @@
>  #include "nsHttpTransaction.h"
>  #include "nsHttpRequestHead.h"
>  #include "nsHttpResponseHead.h"
>  #include "nsHttpChunkedDecoder.h"
> +#include "nsHttpChannel.h"

I'm not seeing any need for this new include

::: netwerk/protocol/http/nsHttpTransaction.h
@@ +26,5 @@
>  class nsIHttpActivityObserver;
>  class nsIEventTarget;
>  class nsIInputStream;
>  class nsIOutputStream;
> +class nsHttpChannel;

I'm not seeing any use of this forward declaration, kill it.

@@ +40,5 @@
>  // intended to run on the socket thread.
>  //-----------------------------------------------------------------------------
>  
>  class nsHttpTransaction : public nsAHttpTransaction
> +                          , public ATokenBucketEvent

You broke the indentation here.

@@ +89,5 @@
>      nsHttpResponseHead    *ResponseHead()   { return mHaveAllHeaders ? mResponseHead : nullptr; }
>      nsISupports           *SecurityInfo()   { return mSecurityInfo; }
>  
>      nsIEventTarget        *ConsumerTarget() { return mConsumerTarget; }
> +    nsIChannel            *HttpChannel()    { return mHttpChannel; }

There doesn't appear to be any reason to have this return an nsIChannel, when what you're returning is an nsIHttpChannel. Change the return value.

@@ +90,5 @@
>      nsISupports           *SecurityInfo()   { return mSecurityInfo; }
>  
>      nsIEventTarget        *ConsumerTarget() { return mConsumerTarget; }
> +    nsIChannel            *HttpChannel()    { return mHttpChannel; }
> +

Kill the extra blank line here.

@@ +194,5 @@
>      nsCOMPtr<nsIAsyncOutputStream>  mPipeOut;
>      nsCOMPtr<nsILoadGroupConnectionInfo> mLoadGroupCI;
>  
>      nsCOMPtr<nsISupports>             mChannel;
> +

Added a blank line for no reason

@@ +199,5 @@
>      nsCOMPtr<nsIHttpActivityObserver> mActivityDistributor;
>  
>      nsCString                       mReqHeaderBuf;    // flattened request headers
>      nsCOMPtr<nsIInputStream>        mRequestStream;
> +    nsCOMPtr<nsIHttpChannel>        mHttpChannel;

I think this will cause a cycle, since the http channel already has a strong ref to the transaction.

Comment 7

[Ben Brittain (:bbrittain)]

Attachment: 1024730-http2

fixed all of the issues, except for the potential cycle. Will think about that tonight.

Comment 8

[Ben Brittain (:bbrittain)]

Attachment: 1024730-http2

Comment 9

[Patrick McManus [:mcmanus]]

Comment on attachment 8465174 [details] [diff] [review]
1024730-http2

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

::: netwerk/base/public/nsIHttpPushListener.idl
@@ +24,5 @@
> +     *                   1 onStop()
> +     *
> +     * as if it were a nsIStreamListener
> +     *
> +     * @param channel

"associatedChannel"

@@ +29,5 @@
> +     *        the monitor channel that was recieved on
> +     * @param pushChannel
> +     *        a channel to the resource which was pushed
> +     */
> +    void onPush(in nsIHttpChannel channel, in nsIHttpChannel pushChannel);

there needs to be an out parameter indicating whether the onPush recipient accepted the push

::: netwerk/base/public/nsNetUtil.h
@@ +41,5 @@
>  #include "nsIStreamLoader.h"
>  #include "nsIUnicharStreamLoader.h"
>  #include "nsIPipe.h"
>  #include "nsIProtocolHandler.h"
> +#include "nsIHttpPushListener.h"

an include without any code that references it is strange..

::: netwerk/protocol/http/Http2Push.cpp
@@ +25,5 @@
>  namespace net {
>  
> +class PushListener MOZ_FINAL : public nsRunnable {
> +  public:
> +    PushListener(nsRefPtr<nsHttpTransaction> trans, nsCString uri):

const nsCString &uri

@@ +26,5 @@
>  
> +class PushListener MOZ_FINAL : public nsRunnable {
> +  public:
> +    PushListener(nsRefPtr<nsHttpTransaction> trans, nsCString uri):
> +      mTrans(trans)

: on this line

@@ +32,5 @@
> +  {
> +  }
> +    NS_IMETHOD Run()
> +    {
> +      NS_ASSERTION(NS_IsMainThread(), "PushListener - Only call on main thread");

trying to just use MOZ_ASSERT now

@@ +36,5 @@
> +      NS_ASSERTION(NS_IsMainThread(), "PushListener - Only call on main thread");
> +
> +      nsCOMPtr<nsIURI> uri;
> +      nsresult rv;
> +      nsHttpChannel* chan = static_cast<nsHttpChannel*>(mTrans->HttpChannel());

that's not safe.. addons can create their own channels, and then hilarity will ensue.

you can actually make it so that you can qi this to an nsHttpChannel directly from the nsIChannel.. nsProxyInfo is a class that does that.

also, it's "type *var" not "type* var" in necko.

@@ +37,5 @@
> +
> +      nsCOMPtr<nsIURI> uri;
> +      nsresult rv;
> +      nsHttpChannel* chan = static_cast<nsHttpChannel*>(mTrans->HttpChannel());
> +      rv = chan->GetURI(getter_AddRefs(uri));

I think you need to move more than the uri.. where are the request headers?

@@ +38,5 @@
> +      nsCOMPtr<nsIURI> uri;
> +      nsresult rv;
> +      nsHttpChannel* chan = static_cast<nsHttpChannel*>(mTrans->HttpChannel());
> +      rv = chan->GetURI(getter_AddRefs(uri));
> +      if (!(NS_FAILED(rv))) {

NS_SUCCESS(rv)

@@ +39,5 @@
> +      nsresult rv;
> +      nsHttpChannel* chan = static_cast<nsHttpChannel*>(mTrans->HttpChannel());
> +      rv = chan->GetURI(getter_AddRefs(uri));
> +      if (!(NS_FAILED(rv))) {
> +        chan->OnPush(mUri);

I'm really confused here at the difference between mURI and the stack uri and what they are used for

@@ +101,5 @@
> +  }
> +
> +  // multiple WriteSegments can occur in a single push,
> +  // no need to notify for uncompleted writes
> +  if (mPushCompleted) {

do you mean !mPushCompleted?

also, I don't think you need to wait for the push to complete.. indeed - it might be flow controlled and will never complete without something accepting the push.

::: netwerk/protocol/http/Http2Push.h
@@ +19,5 @@
>  namespace mozilla {
>  namespace net {
>  
>  class Http2PushTransactionBuffer;
> +class nsIStreamListener;

why?

@@ +61,5 @@
>    nsCOMPtr<nsILoadGroupConnectionInfo> mLoadGroupCI;
>  
>    Http2PushTransactionBuffer *mBufferedPush;
> +
> +  nsAHttpTransaction *mAHttpTransaction;

mAssociatedTransaction

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +41,5 @@
>  #include "mozilla/VisualEventTracer.h"
>  #include "nsISSLSocketControl.h"
>  #include "sslt.h"
> +#include "nsIHttpChannel.h"
> +#include "nsIHttpPushListener.h"

do you need nsIHttpChannel.h?

@@ +66,5 @@
>  #include "nsCRT.h"
>  #include "nsPIDOMWindow.h"
>  #include "nsPerformance.h"
>  #include "CacheObserver.h"
> +#include "Http2Push.h"

why?

@@ +245,5 @@
>  }
>  
> +// Specific to Http2 Code
> +nsresult
> +nsHttpChannel::OnPush(nsCString url)

const nsacstring &url

@@ +267,5 @@
> +        NS_NewChannel(getter_AddRefs(pushChannel), pushResource);
> +        nsCOMPtr<nsIHttpChannel> pushHttpChannel = do_QueryInterface(pushChannel);
> +
> +        // Call the idl code
> +        pushListener->OnPush(this, pushHttpChannel);

so it seems like we are calling onPush with a pushHttpChannel that has only a URI.. What about the request headers? That seems like the important part.

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +238,5 @@
>          mActivityDistributor = nullptr;
>      }
>  
>      nsCOMPtr<nsIChannel> channel = do_QueryInterface(eventsink);
> +    mHttpChannel = do_QueryInterface(eventsink);

let's just QI mChannel to nsIHttpChannelInternal when you need this

Comment 10

[Ben Brittain (:bbrittain)]

Attachment: 1024730-http2

I'm pretty sure I took care of all of the (many) problems. I am a little confused about the nsProxyInfo comment though. It seems to me that I can't do that because ambiguity reasons.

Comment 11

[Patrick McManus [:mcmanus]]

Comment on attachment 8469496 [details] [diff] [review]
1024730-http2

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

I think a handful of simple tests will help you work through the basics of what needs to be supported here.

::: netwerk/base/public/nsIHttpPushListener.idl
@@ +16,5 @@
> + */
> +[scriptable, uuid(0d6ce59c-ad5d-4520-b4d3-09664868f279)]
> +interface nsIHttpPushListener : nsIStreamListener
> +{
> +    /**

total nit - 2 space indent in new code please

@@ +33,5 @@
> +     *        status of whether or not recepient accepted the push
> +     */
> +    void onPush(in nsIHttpChannel associatedChannel,
> +                in nsIHttpChannel pushChannel,
> +                out nsresult accepted);

s/nsresult/bool

::: netwerk/protocol/http/Http2Push.cpp
@@ +25,5 @@
>  namespace net {
>  
> +class PushListener MOZ_FINAL : public nsRunnable {
> +  public:
> +    PushListener(nsRefPtr<nsHttpTransaction> trans, const nsCString &uri,

we don't often deref nsHttpTransaction off the socket thread. Can this class take a nsIHttpChannelInternal reference as an argument instead? Call it associatedChannel

@@ +37,5 @@
> +    {
> +      MOZ_ASSERT(NS_IsMainThread(), "PushListener - Only call on main thread");
> +      nsCOMPtr<nsIChannel> httpChan = do_QueryInterface(mTrans->HttpChannel());
> +      if (httpChan) {
> +        nsHttpChannel *chan = static_cast<nsHttpChannel *>(httpChan.get());

just like last review, this is still not a safe cast. Amend nsIHttpChannelInternal if you need to to let QI do the dynamic cast safely.

@@ +46,5 @@
> +    }
> +
> +  private:
> +    nsRefPtr<nsHttpTransaction> mTrans;
> +    const nsCString mUri;

mURI

@@ +71,5 @@
>    mStreamID = aID;
>    MOZ_ASSERT(!(aID & 1)); // must be even to be a pushed stream
>    mBufferedPush->SetPushStream(this);
>    mLoadGroupCI = aAssociatedStream->LoadGroupConnectionInfo();
> +  mAssociatedTransaction= aAssociatedStream->Transaction();

nit space equals space

@@ +103,5 @@
> +
> +  nsHttpTransaction *trans = mAssociatedTransaction->QueryHttpTransaction();
> +  if (trans) {
> +    nsCString uri = Origin() + Path();
> +    nsHttpHeaderArray requestHeaders = trans->RequestHead()->Headers();

why are you cloning the request headers of the associated transaction in order to notify about the push?

You want the request headers of the pushed channel - not the associated stream request headers. The ones that came in the PUSH_PROMISE h2 frame. You'll see they are pretty much ignored on the stack in the current PUSH_PROMISE handling - but they can easily be added to the http2pushedstream

::: netwerk/protocol/http/Http2Push.h
@@ +60,5 @@
>    nsCOMPtr<nsILoadGroupConnectionInfo> mLoadGroupCI;
>  
>    Http2PushTransactionBuffer *mBufferedPush;
> +
> +  nsAHttpTransaction *mAssociatedTransaction;

I don't think you need this member at all if you change the PushListener object to take a nsIHttpChannelInternal

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +262,5 @@
> +
> +        // Create a Channel for the Push Resource
> +        NS_NewURI(getter_AddRefs(pushResource), url);
> +        NS_NewChannel(getter_AddRefs(pushChannel), pushResource);
> +        nsCOMPtr<nsIHttpChannel> pushHttpChannel = do_QueryInterface(pushChannel);

deal with failures on any of the above 3 things

@@ +274,5 @@
> +        }
> +
> +        // Call the idl code
> +        nsresult result;
> +        pushListener->OnPush(this, pushHttpChannel, &result);

what if OnPush throws?
what if result == false?

more importantly, what happens if it succeeds? The idl says the listener will also get onstart/ondata/onstop.. how are you making that happen (i.e. you need to to an asyncopen()).. 

once started, how will it get the right data? Right now that pushed stream data is just hanging out in session::mStreamTransactionHash.. it will only get hooked up right if the  new stream is a unique URI match - which is fine for browsing but not for your use case. You need to do that matching in this new code somewhere.. probably on the socket thread.

you can see how the current browsing-push code does the lookup in http2stream::parsehttprequestheaders (i.e. it is parsing a new request made by the upper layers of gecko and it is looking to see if there is a pushed stream it can pair with it)

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +13,5 @@
>  #include "nsHttpTransaction.h"
>  #include "nsHttpRequestHead.h"
>  #include "nsHttpResponseHead.h"
>  #include "nsHttpChunkedDecoder.h"
> +#include "nsHttpChannel.h"

dont need this, right?

Comment 12

[Ben Brittain (:bbrittain)]

Attachment: 1024730-http2

not quite a review, since I know it's missing the header information you want included. However, I'm concerned about a couple of things.

I can't figure out what you want from the [out] parameter. It seems like there is no good way of handling this. I'm not sure why the JS would be rejecting pushes if it QIed for them in the first place. Do you want a failure to remove the pushes from the cache? I didn't think this interface should have such power.

also the push request headers won't ever be anything other than the http/1.1-esque :* headers, right? Is there any actual benefit to copying these over to the channel? It's not useful in matching them up with their associated push stream.

Comment 13

[Ben Brittain (:bbrittain)]

JST recommended you take a look at the pretty wonky macro stuff he and I worked out to get this to QI. If you know of a better way of doing this, let me know please!

Comment 14

[Benjamin Smedberg]

What is the compile error for NS_INTERFACE_MAP_ENTRY(nsHttpChannel) ? Is the NS_GET_IID(nsHttpChannel) failing, or is it something about the static_cast?

I also think NS_IMPL_QUERY_INTERFACE_INHERITED would work for this, reduce codesize, and probably make everything a bit faster.

Comment 15

[Ben Brittain (:bbrittain)]

sorry, I wasn't clear, when you try compling with an N_INTERFACE_MAP_ENTRY(nsHttpChannel), you get an ambiguous base problem.

error: ‘nsISupports’ is an ambiguous base of ‘mozilla::net::nsHttpChannel’

Comment 16

[Benjamin Smedberg]

Hrm, that indicates a potential real error in the code. You're reinterpret_casting nsHttpChannel to nsISupports, but there's no real guarantee that nsHttpChannel is binary-compatible with nsISupports.

The only safe way to do this, I think, is to avoid foundInterface entirely and instead do this:

  AddRef();
  *aInstancePtr = this;
foundInterface = reinterpret_cast<nsISupports*>(static_cast<void*>(this));

Comment 17

[Benjamin Smedberg]

Argh, hit enter too soon.

AddRef();
*aInstancePtr = this;
return NS_OK;

That was you're not relying on the C++ ABI to guarantee that because the first base of nsHttpChannel -> HttpBaseChannel -> nsHashPropertyBag -> nsIWritablePropertyBag -> nsISupports that nsHttpChannel* can be safely reinterpret_casted to nsISupports.

Comment 18

[Patrick McManus [:mcmanus]]

(In reply to Ben Brittain (:bbrittain) from comment #12)
> Created attachment 8473750 [details] [diff] [review]
> 1024730-http2
> 
> not quite a review, since I know it's missing the header information you
> want included. However, I'm concerned about a couple of things.
> 
> I can't figure out what you want from the [out] parameter. It seems like
> there is no good way of handling this. I'm not sure why the JS would be
> rejecting pushes if it QIed for them in the first place.

it may selectively reject them on the basis of the uri or the headers.

> Do you want a
> failure to remove the pushes from the cache? I didn't think this interface
> should have such power.
>

yes - it should cancel the stream from the session cache. That's not yet a wohle browser cache - these streams are correlated specifically to the channel that is making the decision.
 
> also the push request headers won't ever be anything other than the
> http/1.1-esque :* headers, right? Is there any actual benefit to copying
> these over to the channel? It's not useful in matching them up with their
> associated push stream.

1] it certainly could be useful to the push consumer - its application specific (your notification application isn't the only user of this)

2] the channel is going to need request headers to make any sense as an http transaction. This is how h2 defines the request headers for it when pushed.

The existing browser cache based code isn't as dependent on the pushed headers because it just services a new request - and that new request has its own request headers. You don't have another request to use like that. (the browser cache code is wrong too - it should be using the pushed headers to find the cache match in the case of Vary)

Comment 19

[Patrick McManus [:mcmanus]]

I'm splitting this into 2 bugs - one for the nsIHttpPushListener and one for Web Push.

Since the comment history of this bug is all about the push listener interface I'm stealing this bug for that purpose and opening 1087823 for web push.

I've updated the patches so we can get this functionality:

* get rid of onPush accept parameter as benjamin told me - just cancel the channel
* attach the push-promise headers to the channel given to onpush
* idl doesn't need to inherit from streamlistener
* simplify include changes
* test coverage
* link onPush to push_promise instead of each data frame
* onpush channels are never orphaned (i.e. timeout)
* prevent onpush channels from going into session cache for unrelated pull
* prevnet pushed streams from being cleaned up while onpush events running
* proper cleanup when onPush related things fail
* integrated benjamins xpcom advice from comment 17
* ridealong altsvc test non default config fix
* inherit callbacks/loagroup from associated channel

Comment 20

[Patrick McManus [:mcmanus]]

Attachment: nsIHttpPushListener

Comment 21

[Patrick McManus [:mcmanus]]

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

Comment 22

[Patrick McManus [:mcmanus]]

Attachment: nsIHttpPushListener

co-author: ben brittain <[e-mail]>

Comment 23

[Patrick McManus [:mcmanus]]

https://tbpl.mozilla.org/?tree=Try&rev=6f1ade0ab002

Comment 24

[u408661]

Comment on attachment 8510782 [details] [diff] [review]
nsIHttpPushListener

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

This LGTM, just a couple nits. It helps having gone through a few rounds prior to this that you didn't even have to write! :-D

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +6505,5 @@
>      return mCacheEntriesToWaitFor != 0;
>  }
>  
> +void
> +nsHttpChannel::SetPushedStream(Http2PushedStream *stream) {

nit: { on new line

::: netwerk/test/unit/test_http2.js
@@ +419,5 @@
> +    var data = read_stream(stream, cnt);
> +
> +    if (ctx.originalURI.spec == "https://localhost:6944/pushapi1") {
> +	do_check_eq(data[0], '0');
> +	-- this.checksPending;

let's get rid of the space before "this"... it made me think I was looking at a SQL comment instead of a variable decrement :) (same for the rest of these, of course)

Comment 25

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/32e40c42dc81

Comment 26

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/32e40c42dc81