1217571 - The imagelib cache is broken in e10s

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[(no longer active)]

The imagelib cache is completely broken as far as I can tell in e10s, in that it seems to be putting things into the cache, and then not using them.

The reason why this happens is that it tries to QI an nsIChannel to an nsICachingChannel to get two bits of information:

* whether the channel is cached (by calling IsFromCache())
* the expiry time of a cache entry (in the beginning of imgRequest::SetCacheValidation())

In the child process, the QIs return null and therefore we can't obtain either of the above bits of info.

Comment 1

[(no longer active)]

Note that I found this by noticing that my test for bug 1206298 is failing in the non-e10s case.  I'm going to land the test there with those checks commented out, please uncomment them when this bug is fixed.  Thanks!

Comment 2

[Jason Duell]

Thanks for noticing this, Ehsan.

The fix is to QI the channel to nsICacheInfoChannel (which has the expiration and isFromCache APIs). 

Historical note: we split out nsICacheInfoChannel as a base class for nsICachingChannel when we did e10s, since we can't support all the APIs in nsICachingChannel on the child.  Child channels only implement nsICacheInfoChannel while parent channels implement the full API.

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: fix the imagelib cache to work in e10s

nsICachingChannel isn't available in the child process; we have to use
nsICacheInfoChannel instead.

The fixes to the service worker tests come from comment 1; I'm assuming that
part is rs=ehsan, since he left a comment about fixing them in the file.

Comment 4

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8686740 [details] [diff] [review]
fix the imagelib cache to work in e10s

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

Nice!

::: dom/workers/test/serviceworkers/test_imagecache_max_age.html
@@ +41,5 @@
>            break;
>          case 2:
>            is(e.data.url, "image-40px.png", "Correct url expected");
>            is(e.data.url2, "image-40px.png", "Correct url expected");
> +          is(e.data.width, 40, "Correct width expected");

Splinter doesn't let me see the whole file, so maybe this is obvious in the broader context, but it might be nice to add a comment here clarifying what this is testing.

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: add a smoketest for the image cache

Comment 6

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8686786 [details] [diff] [review]
add a smoketest for the image cache

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

LGTM!

::: image/test/mochitest/test_bug1217571.html
@@ +25,5 @@
> +  var iframes = document.getElementsByTagName("iframe");
> +  var imgs = Array.from(iframes, function (f) {
> +      return SpecialPowers.wrap(f.contentDocument.getElementsByTagName("img")[0]);
> +  });
> +  var containers = imgs.map(function (img) {

Love it, so functional.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d0012628fce

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2d0012628fce

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Nathan, can you get this uplifted to Aurora?  Seems it might be important for b2g 2.5 (which gets merges from Aurora).

Comment 10

[Nathan Froyd [:froydnj]]

(In reply to Ben Kelly [:bkelly] from comment #9)
> Nathan, can you get this uplifted to Aurora?  Seems it might be important
> for b2g 2.5 (which gets merges from Aurora).

Yes!

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 8686740 [details] [diff] [review]
fix the imagelib cache to work in e10s

This approval request is for both patches; this patch is the real fix, and the other patch is a test to try and ensure we don't regress this again.  (I collapsed them into a single commit for pushing to m-i.)

Approval Request Comment
[Feature/regressing bug #]: Unsure...possibly as long as we have had e10s?
[User impact if declined]: Slower browser, more jank.
[Describe test coverage new/current, TreeHerder]: The imagelib cache is well tested, and this patch just flips it back on.
[Risks and why]: Low risk
[String/UUID change made/needed]: None.

Comment 12

[Jim Mathies [:jimm]]

I think this fixed our tp5o regressions.

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Nathan, just wondering, does the service workers test that was un-commented pass with this fix?

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8686740 [details] [diff] [review]
fix the imagelib cache to work in e10s

This fix has been in Nightly for 3 days, seems safe to uplift to Aurora44. It has also has automated tests which is always great.

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8686786 [details] [diff] [review]
add a smoketest for the image cache

Both patches need to be uplifted. This is a test only patch afaik.

Comment 16

[Carsten Book [:Tomcat]]

(In reply to Ritu Kothari (:ritu) from comment #15)
> Comment on attachment 8686786 [details] [diff] [review]
> add a smoketest for the image cache
> 
> Both patches need to be uplifted. This is a test only patch afaik.

the 2 patches were folded into one checkin.

https://hg.mozilla.org/releases/mozilla-aurora/rev/40f1515c4d05

Comment 17

[Nathan Froyd [:froydnj]]

(In reply to Ritu Kothari (:ritu) from comment #13)
> Nathan, just wondering, does the service workers test that was un-commented
> pass with this fix?

As far as I could tell, it passed with and without this fix; I'm not sure what Ehsan was seeing previously.  It is possible that I got my test invocations mixed up, though...