Closed Bug 1156493 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Networking: Cache, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
e10s m6+ ---
firefox40 --- wontfix
firefox41 --- fixed

People

(Reporter: jduell.mcbugs, Assigned: michal)

References

Details

Attachments

(3 files, 2 obsolete files)

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+
Attached patch patch v1 (obsolete) — Splinter Review
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.
(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).
/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/
Attachment #8599909 - Attachment is obsolete: true
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)
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)
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+
(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.
Attached patch patch v2Splinter Review
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
Closed: 9 years ago
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
Target Milestone: mozilla40 → mozilla41
Blocks: 1163560
Flags: needinfo?(jduell.mcbugs)
Flags: needinfo?(michal.novotny)
Attachment #8599909 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.