Closed Bug 1274516 Opened 3 years ago Closed 3 years ago

The imgICache.clearCache() will not clear image caches for other process in e10s mode

Categories

(Core :: ImageLib, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
e10s + ---
firefox47 --- wontfix
firefox48 + wontfix
firefox49 + fixed
firefox50 + fixed

People

(Reporter: timhuang, Assigned: mrbkap)

References

Details

(Keywords: regression, Whiteboard: [gfx-noted])

Attachments

(3 files, 3 obsolete files)

IIUC there will be two image caches for the content process and the chrome process respectively in e10s mode. If you run the imgICache.clearCache() in one process, it will not clear the other process's cache.
Keywords: regression
OS: Unspecified → All
Hardware: Unspecified → All
Whiteboard: [gfx-noted]
Version: unspecified → Trunk
Blocks: 1238183
tracking-e10s: --- → ?
Seth, is this valid? Is there an image cache in the content process?
Flags: needinfo?(seth)
Hi Seth,
Do you know is there any other bug that was filed and is a duplicate of this one?
Looks like imgLoader is a singleton, so forwarding this over shouldn't be hard. While poking around I noticed some observers that respond to various observers which aren't automatically forwarded to content. Looks like we might need to fix these too.

http://mxr.mozilla.org/mozilla-central/source/image/imgLoader.cpp#1139
http://mxr.mozilla.org/mozilla-central/source/image/imgLoader.cpp#1257

Tim, can you provide any detail about how we use imgCache in content processes? Also do you think this should block e10s rollout?
Flags: needinfo?(tnikkel)
(In reply to Jim Mathies [:jimm] from comment #4)
> Looks like imgLoader is a singleton, so forwarding this over shouldn't be
> hard.

There is the normal loader singleton, and the singleton for private browsing.

The proper way to get an imgCache/imgLoader is to use imgITools::getImgCacheForDocument, but some addons will create their own loader, this is pretty useless though.

> While poking around I noticed some observers that respond to various
> observers which aren't automatically forwarded to content. Looks like we
> might need to fix these too.
> 
> http://mxr.mozilla.org/mozilla-central/source/image/imgLoader.cpp#1139
> http://mxr.mozilla.org/mozilla-central/source/image/imgLoader.cpp#1257
> 
> Tim, can you provide any detail about how we use imgCache in content
> processes? Also do you think this should block e10s rollout?

The image cache is an in-memory cache. Whenever we load an image we go through the image cache, using the result in the cache if it's there and can be used, otherwise putting the result of a new load in the image cache. Image loads use the image cache of the process they happen in.

I don't know offhand the purpose of those observors, and I don't know who the js callers are of clearCache, and what their expectations are for so it's hard for me to comment on the importance of this bug to shipping e10s.
Flags: needinfo?(tnikkel)
Based on the blocking bug (bug 1238183) I guessing this prevents "Forget about this site" from working. User facing features like that seem pretty important and should work in what we ship.
Flags: needinfo?(seth)
(In reply to Timothy Nikkel (:tnikkel) from comment #6)
> Based on the blocking bug (bug 1238183) I guessing this prevents "Forget
> about this site" from working. User facing features like that seem pretty
> important and should work in what we ship.

Following the chain up it looks like this has something to do with ContextualIdentity, which hasn't shipped yet.
Flags: needinfo?(mrbkap)
I've been meaning to do this for a while. We have these nice lambda expressions now, so no need for separately-allocated out-parameter array.
Attachment #8759855 - Flags: review?(wmccloskey)
Attached patch Use the new function (obsolete) — Splinter Review
Here's a use of the new helper!
Attachment #8759856 - Flags: review?(wmccloskey)
I guess lambdas are okay, but could we use an iterator instead? I think the clients would be a lot clearer if they could say:

for (ContentParent* cp = ContentParent::AllProcesses(INCLUDE_DEAD)) {
  ...
}

or something like that. What do you think?
Oops. That should be:

for (ContentParent* cp : ContentParent::AllProcesses(INCLUDE_DEAD)) {
Stylistically I'm pro-lambda. Iterators are significantly more magic. Just one data point; obviously opinions will vary on this issue.
Comment on attachment 8759855 [details] [diff] [review]
Add a helper to iterate over all ContentParents

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

::: dom/ipc/ContentParent.h
@@ +168,5 @@
>  
> +  // Like GetAll, but calls aFunc on all ContentParents instead of returning
> +  // them in a list.
> +  template <class Fn>
> +  static void OnAllParents(Fn& aFunc, bool aIncludeDead)

You should always put the lambda argument last, because it makes callsites much cleaner. See my forthcoming comment on the patch you requested review from me on.
Comment on attachment 8759855 [details] [diff] [review]
Add a helper to iterate over all ContentParents

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

::: dom/ipc/ContentParent.h
@@ +168,5 @@
>  
> +  // Like GetAll, but calls aFunc on all ContentParents instead of returning
> +  // them in a list.
> +  template <class Fn>
> +  static void OnAllParents(Fn& aFunc, bool aIncludeDead)

One other note: for these types of methods, IMO "ForAllParents" is more idiomatic. However, whatever is generally done in DOM code should trump my opinion.

It might be cleaner to use an enum class instead of a boolean argument. I try to minimize boolean arguments these days as enums are a little safer and more readable. Use your best judgement though, I don't have a strong opinion on this - just a suggestion.
Comment on attachment 8759857 [details] [diff] [review]
Clear the image cache in both processes

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

Looks good! Style concerns only.

::: image/imgLoader.cpp
@@ +1350,5 @@
> +    bool privateLoader = this == gPrivateBrowsingLoader;
> +    auto sendClearCache = [ privateLoader, chrome ](ContentParent* cp) {
> +      Unused << cp->SendClearImageCache(privateLoader, chrome);
> +    };
> +    ContentParent::OnAllParents(sendClearCache, /* aIncludeDead = */ false);

If you made the lambda argument to OnAllParents() come last, you could write this as:

ContentParent::OnAllParents(/* aIncludeDead = */ false,
                            [privateLoader, chrome](ContentParent* aCP) {
  Unused << aCP->SendClearImageCache(privateLoader, chrome);
});

This is much more idiomatic.

This isn't DOM code (where I know different rules apply), and given that this is a one line lambda I'd happily r+ (and indeed prefer) this version:

ContentParent::OnAllParents(/* aIncludeDead = */ false, [=](ContentParent* aCP) {
  Unused << aCP->SendClearImageCache(privateLoader, chrome);
});

It's up to you, though.

r+ but I strongly recommend, at a minimum, reversing the order of the arguments to OnAllParents() and using my first suggestion above.
Attachment #8759857 - Flags: review?(seth) → review+
And just imagine:

ContentParent::ForAllParents(ParentSubset::eLive, [=](ContentParent* aCP) {
  Unused << aCP->SendClearImageCache(privateLoader, chrome);
});

Be still my beating heart!
(In reply to Seth Fowler [:seth] [:s2h] from comment #14)
> Stylistically I'm pro-lambda. Iterators are significantly more magic.

How so? Iterators require you to pack up the state needed for iteration: usually just a pointer. Doing it with lambdas, you have to pack up all the state needed by the loop. It pushes the complexity onto the callsites, of which there are a lot more.
Attached patch Use an iteratorSplinter Review
This ended up being a bit more code, but I think it is a little easier at the callsites (with explicit capturing there's a bunch of name duplication).
Attachment #8760473 - Flags: review?(wmccloskey)
Attachment #8759855 - Attachment is obsolete: true
Attachment #8759856 - Attachment is obsolete: true
Attachment #8759857 - Attachment is obsolete: true
Attachment #8759855 - Flags: review?(wmccloskey)
Attachment #8759856 - Flags: review?(wmccloskey)
Attached patch Image cache fixSplinter Review
With that.
Attachment #8760477 - Flags: review?(seth)
Comment on attachment 8760473 [details] [diff] [review]
Use an iterator

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

Thanks!

::: dom/ipc/ContentParent.h
@@ +193,5 @@
> +    }
> +
> +    const ContentParentIterator& operator++()
> +    {
> +      do {

Can you assert mCurrent before the loop?
Attachment #8760473 - Flags: review?(wmccloskey) → review+
Priority: -- → P1
(In reply to Bill McCloskey (:billm) from comment #19)
> (In reply to Seth Fowler [:seth] [:s2h] from comment #14)
> > Stylistically I'm pro-lambda. Iterators are significantly more magic.
> 
> How so? Iterators require you to pack up the state needed for iteration:
> usually just a pointer. Doing it with lambdas, you have to pack up all the
> state needed by the loop. It pushes the complexity onto the callsites, of
> which there are a lot more.

Iterators are much, much harder to look at and say "this code is correct" if the state is more complex than a pointer. I've seen some truly ugly iterator code in my day. =\

Re: packing up all the state needed for the loop, I think actually the opposite is true, but maybe I'm misunderstanding you. The function you pass the lambda to (OnAllParents() in this case) still contains all the iteration-related state, just as in the iterator case, but since it's written in a straight-line style -- as if it was code you had just written at the call site -- it's much easier to understand, verify correctness for, and maintain.

That said, this is pretty much just bikeshedding. It's not important that either of us convince the other, but at least we've been forced to think a little. =)
Comment on attachment 8760477 [details] [diff] [review]
Image cache fix

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

LGTM.
Attachment #8760477 - Flags: review?(seth) → review+
(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #20)
> (with explicit capturing there's a bunch of name duplication).

No kidding. =(
Pushed by mrbkap@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d51d4e1930f
Add a helper to iterate over all ContentParents and use it to clear the image cache in both the parent and child processes. r=billm r=seth
https://hg.mozilla.org/mozilla-central/rev/8d51d4e1930f
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Blake, should we uplift this e10s bug fix from Nightly 50 to Aurora 49 or even Beta 48? We plan to enable e10s by default for some release channel users with Firefox 48.
Flags: needinfo?(mrbkap)
Comment on attachment 8760477 [details] [diff] [review]
Image cache fix

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: We won't clear the cache for certain user-initiated operations in the child process.
[Describe test coverage new/current, TreeHerder]: n/a
[Risks and why]: Low risk; this operation can already be performed in the child process, this just makes us do it more often when asked.
[String/UUID change made/needed]: n/a
Flags: needinfo?(mrbkap)
Attachment #8760477 - Flags: approval-mozilla-release?
Attachment #8760477 - Flags: approval-mozilla-beta?
Comment on attachment 8760477 [details] [diff] [review]
Image cache fix

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

This patch fixes a regression. Take it in 48 beta 5.
Attachment #8760477 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/82eb59127df7

do we want this in aurora too ?
Flags: needinfo?(mrbkap)
Flags: needinfo?(gchang)
Yes. Hi Blake,
Can you create a uplift request for aurora?
Flags: needinfo?(gchang)
Needs bug 1275266 uplifted first, I'd guess.
This should be fine to uplift to aurora, though.
Comment on attachment 8760477 [details] [diff] [review]
Image cache fix

Approval Request Comment: See above. This patch applies as-is.

I can't remove the approval-beta myself, someone else will have to do that. I'll have a beta patch up shortly.
Flags: needinfo?(mrbkap)
Attachment #8760477 - Flags: approval-mozilla-release? → approval-mozilla-aurora?
Attached patch For betaSplinter Review
This was a simple renaming.
Attachment #8767001 - Flags: review+
Comment on attachment 8767001 [details] [diff] [review]
For beta

Approval Request Comment: See above.
Attachment #8767001 - Flags: approval-mozilla-beta?
Hi Blake, 
Do you think if we can let it ride the train on 49 & 50 and won't fix in beta?
Flags: needinfo?(mrbkap)
Hi Chris & Jim,
If this is not in beta, will it be a critical issue? I notice there is a regression in the new patch. What is the impact if we don't fix this in beta and let it ride on 49 and get more testing.
Flags: needinfo?(jmathies)
Flags: needinfo?(cpeterson)
I don't think we need this in Beta 48. Clearing the image cache is probably not a common operation and a regression was already found in the new patch, suggesting there might be other regressions not found yet. I asked about the uplift just to make sure this bug's "status-firefox##" tracking flags were up to date.
Flags: needinfo?(cpeterson)
Yeah, this doesn't need to land on Beta.
Flags: needinfo?(mrbkap)
Flags: needinfo?(jmathies)
Based on comment #41, tracking this for clearing image cache in e10s mode.
Comment on attachment 8767001 [details] [diff] [review]
For beta

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

Based on comment #41 & #42, won't fix in 48 beta.
Attachment #8767001 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment on attachment 8760477 [details] [diff] [review]
Image cache fix

OK for aurora uplift, image cache clearing fix for e10s
Attachment #8760477 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Marking this fix-optional for 49, we could still land it in aurora but I'm also OK with it staying in 50.
(In reply to Carsten Book [:Tomcat] from comment #47)
> backed out for bustage like
> https://treeherder.mozilla.org/logviewer.html#?job_id=2950360&repo=mozilla-
> aurora

It looks like the ContentParent changes didn't land in that push. Given that we're calling this fix-optional, let's just let this patch ride the trains.
Flags: needinfo?(mrbkap)
You need to log in before you can comment on or make changes to this bug.