697392 - e10s HTTP: Correctly deliver on-examine-{cached,merged}-response notifications on child

Status: RESOLVED WONTFIX

(Core :: Networking: HTTP, defect)

Description

[Jason Duell]

Attachment: Correctly deliver on-examine-{cached,merged}-response notifications on child.

Right now we always notify on-examine-response observers on the child.  That's incorrect.  Attached patch figures out whether to call on-examine-{cached|merged}-response observers instead.

Bjarne:  do you have any opinion on how to test this?  The existing xpcshell test for this (test_bug482601.js) can't run in the child, as it uses nsICacheService, which is not available there.   If you don't have any ideas, I'd vote to just load a URI twice, and expect on-response the first time and on-cached-response the 2nd, and just skip on-merged testing unless you can figure out a good way to simulate it.  This code is unlikely to be wrong anyway.

If you don't know e10s well enough to review this quickly, feel free to reassign to Honza or Michal.

Comment 1

[Bjarne (:bjarne)]

(In reply to Jason Duell (:jduell) from comment #0)
> I'd vote to just load a URI twice, and expect on-response the first
> time and on-cached-response the 2nd, and just skip on-merged testing unless
> you can figure out a good way to simulate it.

I think your proposed approach is ok. Assuming "on-merged" means loading a partly cached entry, have a look at "test_gzipped_206.js". It's cachedHandler() returns a partial entry on first call and the rest on second call.

> This code is unlikely to be wrong anyway.

Hehe... never heard that one before... :-D

> If you don't know e10s well enough to review this quickly, feel free to
> reassign to Honza or Michal.

I suggest to reassign if you need a quick review - I don't know e10s particularly well.

Comment 2

[Bjarne (:bjarne)]

Comment on attachment 569630 [details] [diff] [review]
Correctly deliver on-examine-{cached,merged}-response notifications on child.

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

Reassigning to Honza who know this stuff a lot better than I.

Comment 3

[Honza Bambas (:mayhemer)]

Comment on attachment 569630 [details] [diff] [review]
Correctly deliver on-examine-{cached,merged}-response notifications on child.

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

r-

mIsFromCache may not be always set correctly on the child.  You have to carry both the notification type and the information of isFromCache.  e.g. in case of 304 you will report as not from cache.

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +286,5 @@
>      NS_DeserializeObject(securityInfoSerialization, 
>                           getter_AddRefs(mSecurityInfo));
>    }
>  
> +  mIsFromCache = (onExamineType == examine_cached ? true : false);

isn't |onExamineType == examine_cached| it self enough?  However, this code is wrong anyway.

::: netwerk/protocol/http/PHttpChannel.ipdl
@@ +128,5 @@
>  child:
>    OnStartRequest(nsHttpResponseHead  responseHead,
>                   bool                useResponseHead,
>                   RequestHeaderTuples requestHeaders,
> +                 int                 onExamineType,

I don't see a reason not to rather use PRUint32 here.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +270,5 @@
>          if (mCachedContentIsValid) {
>              nsRunnableMethod<nsHttpChannel> *event = nsnull;
>              if (!mCachedContentIsPartial) {
> +                NS_ASSERTION(mOnExamineType == examine_invalid,
> +                            "mOnExamineType already set");

Indention.

Comment 4

[atifrea]

Attachment: Bug 697392 - Patch v2: Fixed the nits

Comment 5

[Honza Bambas (:mayhemer)]

Comment on attachment 8452553 [details] [diff] [review]
Bug 697392 - Patch v2: Fixed the nits

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

Please resubmit the patch with 8 line context.

Comment 6

[atifrea]

Attachment: Bug 697392 - Patch v2: Fixed the nits

Comment 7

[Honza Bambas (:mayhemer)]

Comment on attachment 8453561 [details] [diff] [review]
Bug 697392 - Patch v2: Fixed the nits

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

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +185,5 @@
>                      const nsHttpResponseHead& responseHead,
>                      const bool& useResponseHead,
>                      const nsHttpHeaderArray& requestHeaders,
>                      const bool& isFromCache,
> +                    const int& onExamineType,

uint32_t

@@ +222,5 @@
>    nsHttpResponseHead mResponseHead;
>    nsHttpHeaderArray mRequestHeaders;
>    bool mUseResponseHead;
>    bool mIsFromCache;
> +  int mOnExamineType;

uint32_t

@@ +237,5 @@
>                                       const nsHttpResponseHead& responseHead,
>                                       const bool& useResponseHead,
>                                       const nsHttpHeaderArray& requestHeaders,
>                                       const bool& isFromCache,
> +                                     const int& onExamineType,

uint32_t

@@ +279,5 @@
>                                   const nsHttpResponseHead& responseHead,
>                                   const bool& useResponseHead,
>                                   const nsHttpHeaderArray& requestHeaders,
>                                   const bool& isFromCache,
> +                                 const int& onExamineType,

uint32_t onExamineResponseNotifyType

@@ +308,5 @@
>      NS_DeserializeObject(securityInfoSerialization,
>                           getter_AddRefs(mSecurityInfo));
>    }
>  
> +  mIsFromCache = isFromCache || (onExamineType == examine_cached);

don't change this.

::: netwerk/protocol/http/HttpChannelChild.h
@@ +100,5 @@
>                            const nsHttpResponseHead& responseHead,
>                            const bool& useResponseHead,
>                            const nsHttpHeaderArray& requestHeaders,
>                            const bool& isFromCache,
> +                          const int& onExamineType,

uint32_t

@@ +169,5 @@
>                        const nsHttpResponseHead& responseHead,
>                        const bool& useResponseHead,
>                        const nsHttpHeaderArray& requestHeaders,
>                        const bool& isFromCache,
> +                      const int& onExamineType,

uint32_t

::: netwerk/protocol/http/PHttpChannel.ipdl
@@ +84,5 @@
>                   nsHttpResponseHead  responseHead,
>                   bool                useResponseHead,
>                   nsHttpHeaderArray   requestHeaders,
>                   bool                isFromCache,
> +                 PRUint32            onExamineType,

s/PRUint32/uint32_t/

onExamineResponseNotifyType

::: netwerk/protocol/http/nsHttp.h
@@ +49,5 @@
> +    examine_invalid = 0,
> +    examine_response,
> +    examine_cached,
> +    examine_merged
> +};

enum EHttpOnExamineResponseType {
  HTTP_ON_EXAMINE_RESPONSE_UNKNOWN = 0,
  HTTP_ON_EXAMINE_RESPONSE,
  HTTP_ON_EXAMINE_CACHED_RESPONSE,
  HTTP_ON_EXAMINE_MERGED_RESPONSE
};

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +361,5 @@
>          if (mCachedContentIsValid) {
>              nsRunnableMethod<nsHttpChannel> *event = nullptr;
>              if (!mCachedContentIsPartial) {
> +                NS_ASSERTION(mOnExamineType == examine_invalid,
> +                            "mOnExamineType already set");

indention

@@ +2339,5 @@
> +    // notify observers interested in looking at a response that has been
> +    // merged with any cached headers (http-on-examine-merged-response).
> +    NS_ASSERTION(mOnExamineType == examine_invalid,
> +                 "mOnExamineType already set");
> +    mOnExamineType = examine_merged;

I'm afraid one of these assertions will fail.  We may notify on-examine-response and then on-examine-merged-response - at least I think.

This may also kinda complicate the whole work here...  I'm not sure (someone has to check it) but we may need a special IPC to notify merged/cached response.

::: netwerk/protocol/http/nsHttpChannel.h
@@ +139,5 @@
>          mRequestHead.SetHeader(nsHttp::Referer, spec);
>          return NS_OK;
>      }
>  
> +    int GetOnExamineType() { return mOnExamineType; }

s/int/uint32_t/

GetOnExamineResponseNotifyType() const

@@ +391,5 @@
>      // True if mRequestTime has been set. In such a case it is safe to update
>      // the cache entry's expiration time. Otherwise, it is not(see bug 567360).
>      uint32_t                          mRequestTimeInitialized : 1;
> +    // Tells e10s child which type of on-examine- notification to send (see bug 697392)
> +    PRUint32                          mOnExamineType            : 2;

s/PRUint32/uint32_t/

mOnExamineResponseType

put at the end of all bitfield members

Comment 8

[atifrea]

(In reply to Honza Bambas (:mayhemer) from comment #7)
> I'm afraid one of these assertions will fail.  We may notify
> on-examine-response and then on-examine-merged-response - at least I think.
> 
> This may also kinda complicate the whole work here...  I'm not sure (someone
> has to check it) but we may need a special IPC to notify merged/cached
> response.

Hi,

I think that you might be right about this (once mOnExamineType is set, I don't think it is ever reset to its default value). But I didn't understand from your comment when this can happen and what the solution for this is. Could you please explain this?. Thanks.

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

jduell, do we still care about this?

Comment 10

[Jason Duell]

Brad,

So far we've managed to get away without needing these notifications.  I think the question here is whether we're planning to run addon code on child process (or if they will always stay on the parent).  If there's addon code on the child we should probably be delivering these for compat sake.

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

We're handling these sort of compat issues with the addon shims. Bill, do we need to do something here?

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I don't think these notifications will help us. Any add-on code running in the child process is e10s-aware, so we don't have to worry about compat.