722861 - imgLoader uses global PB notifications to clear the image cache

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Josh Matthews [:jdm]]

In the brave new world of per-window private browsing, there will probably be no need to do this. Instead, there will be a separate PB cache that will not need clearing.

Comment 1

[Josh Matthews [:jdm]]

Attachment: Add privacy information to image requests, and use a separate cache for private requests.

Comment 2

[Josh Matthews [:jdm]]

Attachment: Add privacy information to image requests, and use a separate cache for private requests.

Comment 3

[Josh Matthews [:jdm]]

Attachment: Add privacy information to image requests, and use a separate cache for private requests.

Comment 4

[Mozilla RelEng Bot]

Try run for fb942fcbfa73 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=fb942fcbfa73
Results (out of 15 total builds):
    exception: 14
    success: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-fb942fcbfa73

Comment 5

[Mozilla RelEng Bot]

Try run for c4180159869b is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=c4180159869b
Results (out of 175 total builds):
    exception: 6
    success: 104
    warnings: 64
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-c4180159869b

Comment 6

[Mozilla RelEng Bot]

Try run for dd9dfd7aa756 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=dd9dfd7aa756
Results (out of 177 total builds):
    success: 159
    warnings: 18
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-dd9dfd7aa756

Comment 7

[(no longer active)]

Is this ready for review?

Comment 8

[Josh Matthews [:jdm]]

No; I want to write tests first to ensure this is doing what I think it is.

Comment 9

[Josh Matthews [:jdm]]

Attachment: Bug - Tests for imgLoader privacy-respecting changes.

Comment 10

[Josh Matthews [:jdm]]

Attachment: Add privacy information to image requests, and use a separate cache for private requests.

Comment 11

[Josh Matthews [:jdm]]

Comment on attachment 614149 [details] [diff] [review]
Bug  - Tests for imgLoader privacy-respecting changes.

These tests of loadImage and loadImageWithChannel pass with the other patch applied. Pray give me your thoughts; the changes to ImageListener and ChannelListener are there because it is the most frustrating thing when errors are silently discarded.

Comment 12

[Josh Matthews [:jdm]]

Comment on attachment 614150 [details] [diff] [review]
Add privacy information to image requests, and use a separate cache for private requests.

I wish your thorough review on these changes. This is practically my first time touching anything in image/, so there was a lot of fumbling around. Specifically, please keep Honza's thoughts from bug 722845 comment 60 in mind - I tried to structure my changes (and the tests) to resolve the problems he describes.

Comment 13

[Josh Matthews [:jdm]]

The comment in http://mxr.mozilla.org/mozilla-central/source/image/src/imgLoader.cpp#524 worries me, since the loadgroup we grab could make a difference. Thoughts?

Comment 14

[Josh Matthews [:jdm]]

Comment on attachment 614150 [details] [diff] [review]
Add privacy information to image requests, and use a separate cache for private requests.

>+  nsCOMPtr<nsILoadContext> loadContext = do_GetInterface(loadGroup);

This is incorrect; I need to obtain the notification callbacks and use getInterface on those instead.

Comment 15

[Justin Lebar (not reading bugmail)]

After skimming the patches, my first reaction is to point out that I have a personal vendetta against boolean parameters in public functions [1].  :)

This case is tricky because you use the boolean in so many places, and you're often passing a boolean down from one function to another.  If you used an enum here, it's not clear to me where we'd want to define said enum.  I'm not sure what to do here, but maybe you can come up with a clean way to do it...

[1] http://jlebar.com/2011/12/16/Boolean_parameters_to_API_functions_considered_harmful..html

Comment 16

[Justin Lebar (not reading bugmail)]

I really don't like the fact that this patch adds an argument we have to explicitly pass around everywhere (boolean or otherwise).  In this new world, we have to explicitly be aware of PB state whenever we touch this code, whereas before, PB implicitly worked.

It looks to me like you're logically making two separate image caches, one for PB and one for regular browsing.  Rather than put those on the same object and multiplex between the two with a boolean on the imgICache methods, perhaps we could leave the imgICache interface alone and have two separate imgLoader objects.

We could have a helper in nsContentUtils (or whatever) which takes a document or docshell and returns the appropriate imgLoader.

Dictating specific design is almost always a bad idea, so feel free to ignore these specifics if you don't think they will work.  It's the general idea that PB should be implicit state that I'd like us to try to figure out.

Comment 17

[Justin Lebar (not reading bugmail)]

Comment on attachment 614149 [details] [diff] [review]
Bug  - Tests for imgLoader privacy-respecting changes.

+function run_loadImage_tests() {
+  clearAllImageCaches();
+  let cs = Cc["@mozilla.org/network/cache-service;1"].getService(Ci.nsICacheService);
+  cs.evictEntries(Ci.nsICache.STORE_ANYWHERE);
+
+  loadImage(false, function() {
+    loadImage(false, function() {
+      loadImage(true, function() {
+        loadImage(true, function() {
+          do_check_eq(gHits, 4);
+          do_test_finished();
+        });
+      });
+    });
+  });
+}

I don't get why this is 4 and not 2, but I imagine I'm missing something here. It looks good aside from this.

Comment 18

[Josh Matthews [:jdm]]

(In reply to Justin Lebar [:jlebar] from comment #17)
> Comment on attachment 614149 [details] [diff] [review]
> Bug  - Tests for imgLoader privacy-respecting changes.
> 
> +function run_loadImage_tests() {
> +  clearAllImageCaches();
> +  let cs =
> Cc["@mozilla.org/network/cache-service;1"].getService(Ci.nsICacheService);
> +  cs.evictEntries(Ci.nsICache.STORE_ANYWHERE);
> +
> +  loadImage(false, function() {
> +    loadImage(false, function() {
> +      loadImage(true, function() {
> +        loadImage(true, function() {
> +          do_check_eq(gHits, 4);
> +          do_test_finished();
> +        });
> +      });
> +    });
> +  });
> +}
> 
> I don't get why this is 4 and not 2, but I imagine I'm missing something
> here. It looks good aside from this.

This is because I don't reset gHits. I'll probably do that to make the test clearer.

Comment 19

[Josh Matthews [:jdm]]

Attachment: Add privacy information to image requests, and use a separate cache for private requests. WIP

Comment 20

[Josh Matthews [:jdm]]

I still need to sort out the following issues:
* memory reporting
* init/teardown
* exposing an API to JS that returns the proper image loader, similar to nsContentUtils
* investigate addon usage of the imgILoader service

However, the approach you suggested looks pretty good to me. I stuck in asserts to LoadImage and LoadImageWithChannel to crash and burn if a private loader attempts to load a non-private request and vice-versa.

Comment 21

[Mozilla RelEng Bot]

Try run for f9d538b51a57 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f9d538b51a57
Results (out of 42 total builds):
    exception: 11
    success: 1
    warnings: 26
    failure: 4
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f9d538b51a57

Comment 22

[Josh Matthews [:jdm]]

Comment on attachment 622650 [details] [diff] [review]
Add privacy information to image requests, and use a separate cache for private requests. WIP

Actually this patch is quite broken in its current form. I'll keep working on it.

Comment 23

[Mozilla RelEng Bot]

Try run for 406a7a7f69c6 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=406a7a7f69c6
Results (out of 137 total builds):
    exception: 40
    success: 8
    warnings: 81
    failure: 8
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-406a7a7f69c6

Comment 24

[Josh Matthews [:jdm]]

Attachment: Add privacy information to image requests, and use a separate cache for private requests.

This patch seems to be passing all tests except for the 404 notification xpcshell one. I'm investigating.

Comment 25

[Josh Matthews [:jdm]]

Comment on attachment 627178 [details] [diff] [review]
Add privacy information to image requests, and use a separate cache for private requests.

Still, this is as good a time as any for your thoughts, Justin.

Comment 26

[Justin Lebar (not reading bugmail)]

(In reply to Josh Matthews [:jdm] (travelling until June 25th) from comment #25)
> Still, this is as good a time as any for your thoughts, Justin.

I think this is the right idea!

A few things offhand that I'd probably change:

> +  if (!nsContentUtils::GetImgLoaderForChannel(nsnull))

It's not clear to me that we can ever be unable to get an image loader; if not, then we should just get rid of these branches.

But if it is possible that we can't get an image loader, then we should have a ::HasImageLoaders() or something method.  Then we can make GetImgLoaderForChannel crash on null...

> -  nsCOMPtr<imgILoader> loader = do_GetService("@mozilla.org/image/loader;1",
> -                                              &rv);
> +  nsCOMPtr<imgILoader> loader = nsContentUtils::GetImgLoaderForDocument(document);
>    if (NS_FAILED(rv)) return rv;

I think you mean |NS_ENSURE_STATE(loader)|.  :)

Comment 27

[Josh Matthews [:jdm]]

Attachment: Bug - Tests for imgLoader privacy-respecting changes.

Comment 28

[Josh Matthews [:jdm]]

Attachment: Add privacy information to image requests, and use a separate cache for private requests.

Comment 29

[Joe Drew (not getting mail)]

Comment on attachment 636592 [details] [diff] [review]
Add privacy information to image requests, and use a separate cache for private requests.

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

Add {} to if bodies throughout, please

Also: oh man am I excited that this removes the statically-allocated caches. bug 529949 fixed too!

::: content/base/src/nsContentUtils.cpp
@@ +518,5 @@
>  
>    // Ignore failure and just don't load images
> +  nsresult rv = CallCreateInstance("@mozilla.org/image/loader;1", &sImgLoader);
> +  if (NS_SUCCEEDED(rv))
> +    rv = CallCreateInstance("@mozilla.org/image/loader;1", &sPrivateImgLoader);

I'm not convinced we need to handle failure here. I'd add an NS_ABORT_IF_FALSE(NS_SUCCEEDED(rv)), and then just assume success afterwards.

@@ +531,5 @@
> +      if (sPrivateImgCache)
> +        sPrivateImgCache->RespectPrivacyNotifications();
> +    }
> +    if (NS_FAILED(rv))
> +      sImgCache = sPrivateImgCache = nsnull;

Similarly here.

@@ +2649,5 @@
> +  return isPrivate ? sPrivateImgLoader : sImgLoader;
> +}
> +
> +// static
> +imgILoader*

Should this return an already_AddRefed? (Honest question - at one point, the answer was definitely 'yes'.)

(Same thing applies to GetImgLoaderForDocument)

@@ +2667,5 @@
>          InitImgLoader();
>  
> +    imgILoader* loader = GetImgLoaderForDocument(aDocument);
> +    nsCOMPtr<imgICache> cache = do_QueryInterface(loader);
> +    if (!cache) return false;

Where you run across single-line if mistakes like this, can you correctly put the body on its own line?

Or, since this is an entirely useless check, you can remove it :)

::: image/public/imgICache.idl
@@ +51,5 @@
>    nsIProperties findEntryProperties(in nsIURI uri);
> +
> +  /**
> +   * Make this cache instance respect private browsing notifications by clearing
> +   * the contents.

The ways this differs from clearCache should be better documented!

::: image/src/imgLoader.cpp
@@ +831,2 @@
>  {
> +  NS_ADDREF(sMemReporter);

You should either do sMemReporter->AddRef() here or NS_RELEASE() below.

@@ +1593,5 @@
>  
>    nsresult rv;
>    nsLoadFlags requestFlags = nsIRequest::LOAD_NORMAL;
>  
> +  nsCOMPtr<nsIPrivateBrowsingConsumer> pbConsumer = do_QueryInterface(aRequest);

Should all of this be #ifdef DEBUG?

::: image/src/imgRequest.cpp
@@ +79,5 @@
>                     nsIInterfaceRequestor,
>                     nsIAsyncVerifyRedirectCallback)
>  
> +imgRequest::imgRequest(imgLoader* aLoader) :
> +mLoader(aLoader), mValidator(nsnull), mImageSniffers("image-sniffing-services"),

indent here

::: image/src/imgRequest.h
@@ +183,5 @@
>  
>  private:
>    friend class imgMemoryReporter;
>  
> +  imgLoader* mLoader;

This should be documented as a weak reference, and that this imgRequest can't outlive its loader.

Comment 30

[Josh Matthews [:jdm]]

https://tbpl.mozilla.org/?tree=Try&rev=764abd7b176f

Comment 31

[Josh Matthews [:jdm]]

https://tbpl.mozilla.org/?tree=Try&rev=db869f1ade36

Comment 32

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c3333e6e37aa
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b4ab96903f2

Comment 33

[Josh Matthews [:jdm]]

ttps://hg.mozilla.org/integration/mozilla-inbound/rev/da1ba554adb8

Comment 34

[Josh Matthews [:jdm]]

https://tbpl.mozilla.org/?tree=Try&rev=4d699eb5288d

Comment 35

[Josh Matthews [:jdm]]

Attachment: Add privacy information to image requests, and use a separate cache for private requests.

Comment 36

[Josh Matthews [:jdm]]

https://tbpl.mozilla.org/?tree=Try&rev=6ae02e6d2b9c

Comment 37

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/08922991ac7d
https://hg.mozilla.org/integration/mozilla-inbound/rev/bdc68f5ddb38

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/08922991ac7d
https://hg.mozilla.org/mozilla-central/rev/bdc68f5ddb38

Comment 39

[Josh Matthews [:jdm]]

Based on the experience of bug 789546, we need to do our best to ensure that extension authors no longer try to get the imgICache or imgILoader interface as a service and use the new methods in imgITools instead.

Comment 40

[Josh Matthews [:jdm]]

If comment 39 is something we could statically check for with the magic tools we possess, that would be super fantastic.

Comment 41

[Josh Matthews [:jdm]]

With regards to addon compatibility, I updated the docs at https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/imgICache to reflect the relevant changes. See the big red box there.