e10s: move .cacheKey to nsICacheInfoChannel so child channels can get/set it

RESOLVED FIXED in Firefox 41

Status

()

Core
Networking: Cache
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jduell, Assigned: michal)

Tracking

unspecified
mozilla41
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10sm6+, firefox40 wontfix, firefox41 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 2 obsolete attachments)

For bug 1025146 we need to let the docshell in e10s child processes get/set the .cacheKey.  Right now .cacheKey lives in nsICachingChannel, which exists only on the parent process.

I don't know this code well, but from a glance it looks like we'd only need to ship one more argument (the cacheKey) over to the child during SendOnStartRequest, and also during AsyncOpen (in case the key has been set on the child).
This is valid, but will probably not solve bug 1025146.
I hope this isn't too presumptuous. :D
Assignee: nobody → michal.novotny
tracking-e10s: --- → m6+
(Assignee)

Comment 3

3 years ago
Created attachment 8596002 [details] [diff] [review]
patch v1
Attachment #8596002 - Flags: review?(jduell.mcbugs)
I have applied this patch in my queue and can confirm that this will address bug 1025146 along with the patches I'm writing to load the source in a remote browser.

Comment 5

3 years ago
(In reply to Honza Bambas from comment #1)
> This is valid, but will probably not solve bug 1025146.

On the other hand it should make it possible for Session Restore to restore a POSTed page correctly under e10s, which I can only assume it hasn't been doing.
Just a gentle review ping here. This is blocking two bugs that are part of e10s "milestone 6", which we'd really like to have wrapped up before the next uplift (May 11th).
Created attachment 8599909 [details]
MozReview Request: bz://1156493/mconley

/r/7639 - Bug 1156493 - "e10s: move .cacheKey to nsICacheInfoChannel so child channels can get/set it" [r=michal.novotny]
/r/7641 - Bug 1128050 - [e10s] Make it possible to save pages that were reached via a POST request. r=?

Pull down these commits:

hg pull -r 63af65ab1cea5d77acebf7808d6dbce3ab8c96cc https://reviewboard-hg.mozilla.org/gecko/
Comment on attachment 8599909 [details]
MozReview Request: bz://1156493/mconley

/r/7639 - Bug 1156493 - "e10s: move .cacheKey to nsICacheInfoChannel so child channels can get/set it" [r=michal.novotny]
/r/7641 - Bug 1128050 - [e10s] Make it possible to save pages that were reached via a POST request. r=?

Pull down these commits:

hg pull -r 2ef04bc74e84fba70f88c49d4ba75f7304949645 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8599909 - Attachment is obsolete: false
Comment on attachment 8599909 [details]
MozReview Request: bz://1156493/mconley

/r/7639 - Bug 1156493 - "e10s: move .cacheKey to nsICacheInfoChannel so child channels can get/set it" [r=michal.novotny]
/r/7641 - Bug 1128050 - [e10s] Make it possible to save pages that were reached via a POST request. r=?

Pull down these commits:

hg pull -r 2ef04bc74e84fba70f88c49d4ba75f7304949645 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8599909 - Flags: review?(wmccloskey)
Comment on attachment 8599909 [details]
MozReview Request: bz://1156493/mconley

/r/7639 - Bug 1156493 - "e10s: move .cacheKey to nsICacheInfoChannel so child channels can get/set it" [r=michal.novotny]
/r/7641 - Bug 1128050 - [e10s] Make it possible to save pages that were reached via a POST request. r=?

Pull down these commits:

hg pull -r 2ef04bc74e84fba70f88c49d4ba75f7304949645 https://reviewboard-hg.mozilla.org/gecko/
Comment on attachment 8599909 [details]
MozReview Request: bz://1156493/mconley

/r/7639 - Bug 1156493 - "e10s: move .cacheKey to nsICacheInfoChannel so child channels can get/set it" [r=michal.novotny]
/r/7641 - Bug 1128050 - [e10s] Save page as... doesn't always load from cache. r=?

Pull down these commits:

hg pull -r bda4c1f32026b75c57c742d6ea8ff0e195ad0a99 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8599909 - Flags: review?(wmccloskey)
Comment on attachment 8599909 [details]
MozReview Request: bz://1156493/mconley

/r/7639 - Bug 1156493 - "e10s: move .cacheKey to nsICacheInfoChannel so child channels can get/set it" [r=michal.novotny]
/r/7641 - Bug 1128050 - [e10s] Save page as... doesn't always load from cache. r=?

Pull down these commits:

hg pull -r bda4c1f32026b75c57c742d6ea8ff0e195ad0a99 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8599909 - Flags: review?(wmccloskey)
(Reporter)

Comment 13

3 years ago
Comment on attachment 8596002 [details] [diff] [review]
patch v1

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

Just a few nits--no need for re-review as long as you're ok with the GetData() cleanup, and the ENSURE_CALLED_BEFORE_ASYNC_OPEN.  The QI call cleanup I might be wrong about, and/or doesn't matter much either way.

Nice patch!

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +448,5 @@
>    mCachedCharset = cachedCharset;
>  
> +  nsRefPtr<nsHttpChannelCacheKey> tmpKey = new nsHttpChannelCacheKey();
> +  tmpKey->SetData(cacheKey.postId(), cacheKey.key());
> +  CallQueryInterface(tmpKey.get(), getter_AddRefs(mCacheKey));

Any reason for the CallQI() call instead of something like this?

   mCacheKey = do_QueryInterface(tmpKey.get())

@@ +1670,5 @@
> +
> +    if (NS_FAILED(postIdObj->GetData(&postId)) ||
> +        NS_FAILED(keyObj->GetData(key))) {
> +      return NS_ERROR_ILLEGAL_VALUE;
> +    }

All this QI'ing to nsISupportsPRUint32, etc, seems like a lot of work for no good reason?  Seems like it would be fewer lines of code (and easier to understand) if we just add a GetData(postId&, key&) accessor to the nsHttpChannelCacheKey class, and then it's a one-liner.

@@ +1828,5 @@
> +}
> +NS_IMETHODIMP
> +HttpChannelChild::SetCacheKey(nsISupports *cacheKey)
> +{
> +  NS_ENSURE_TRUE(!mWasOpened, NS_ERROR_ALREADY_OPENED);

Use the ENSURE_CALLED_BEFORE_ASYNC_OPEN() macro here--it gives us a fatal error in a debug build (instead of a warning no one will probably ever see).

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +394,5 @@
> +    nsRefPtr<nsHttpChannelCacheKey> cacheKey = new nsHttpChannelCacheKey();
> +    cacheKey->SetData(aCacheKey.get_HttpChannelCacheKey().postId(),
> +                      aCacheKey.get_HttpChannelCacheKey().key());
> +    nsCOMPtr<nsISupports> cacheKeySupp;
> +    CallQueryInterface(cacheKey.get(), getter_AddRefs(cacheKeySupp));

Again, possible to make one-liner?

   nsCOMPtr<nsISupports> cacheKeySupp = do_QI(cacheKey.get())

@@ +828,5 @@
> +    }
> +
> +    if (NS_FAILED(postIdObj->GetData(&postId)) ||
> +        NS_FAILED(keyObj->GetData(key))) {
> +      return NS_ERROR_ILLEGAL_VALUE;

See my other comment on adding a better GetData() accessor here, to avoid all the QIing you're doing here.
Attachment #8596002 - Flags: review?(jduell.mcbugs) → review+
(Assignee)

Comment 14

3 years ago
(In reply to Jason Duell [:jduell] (needinfo? me) from comment #13)
> @@ +1670,5 @@
> > +
> > +    if (NS_FAILED(postIdObj->GetData(&postId)) ||
> > +        NS_FAILED(keyObj->GetData(key))) {
> > +      return NS_ERROR_ILLEGAL_VALUE;
> > +    }
> 
> All this QI'ing to nsISupportsPRUint32, etc, seems like a lot of work for no
> good reason?  Seems like it would be fewer lines of code (and easier to
> understand) if we just add a GetData(postId&, key&) accessor to the
> nsHttpChannelCacheKey class, and then it's a one-liner.

cacheKey is now implemented by http channel only, so it would work. But is it really OK to do static_cast<nsHttpChannelCacheKey *> on nsISupports when it could be in theory some other class?
Flags: needinfo?(jduell.mcbugs)
fwiw, I've definitely seen the pattern of static_cast'ing an nsISupports* to a concrete implementation in various parts of Gecko as a way of passing things through XPConnect without adding a new IDL.
(Assignee)

Comment 16

3 years ago
Created attachment 8603044 [details] [diff] [review]
patch v2

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f792a883c42


> ::: netwerk/protocol/http/HttpChannelChild.cpp
> > +  CallQueryInterface(tmpKey.get(), getter_AddRefs(mCacheKey));
> 
> Any reason for the CallQI() call instead of something like this?
> 
>    mCacheKey = do_QueryInterface(tmpKey.get())

Compiler complains that nsISupports is an ambiguous base. I like more CallQueryInterface() instead of this:

mCacheKey = do_QueryInterface(static_cast<nsISupportsPRUint32 *>(tmpKey.get()));
Attachment #8596002 - Attachment is obsolete: true
Attachment #8603044 - Flags: review+
I'm getting kinda worried with how close we're cutting it to the merge with this patch here. Are we certain this will land in time to land the things it blocks before the cutoff?
Flags: needinfo?(michal.novotny)
https://hg.mozilla.org/mozilla-central/rev/371cbdcc9562
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Depends on: 1164318
This was backed out of mozilla-aurora for causing bug 1163900. There's another fix in that bug that we've landed on integration, but it was deemed too risky to take to aurora. The e10s and Necko team are fine with backing this out of 40.

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/3d8623ee8634
status-firefox40: fixed → wontfix
status-firefox41: --- → fixed
Target Milestone: mozilla40 → mozilla41

Updated

3 years ago
Blocks: 1163560
(Reporter)

Updated

3 years ago
Flags: needinfo?(jduell.mcbugs)
(Assignee)

Updated

3 years ago
Flags: needinfo?(michal.novotny)
Comment on attachment 8599909 [details]
MozReview Request: bz://1156493/mconley
Attachment #8599909 - Attachment is obsolete: true
Created attachment 8620096 [details]
MozReview Request: Bug 1128050 - [e10s] Save page as... doesn't always load from cache. r=?
Created attachment 8620097 [details]
MozReview Request: Bug 1156493 - "e10s: move .cacheKey to nsICacheInfoChannel so child channels can get/set it" [r=michal.novotny]
You need to log in before you can comment on or make changes to this bug.