Last Comment Bug 722861 - imgLoader uses global PB notifications to clear the image cache
: imgLoader uses global PB notifications to clear the image cache
Status: RESOLVED FIXED
: addon-compat, dev-doc-needed
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla18
Assigned To: Josh Matthews [:jdm]
:
Mentors:
Depends on: 722845 725210 789546 789909 803150
Blocks: PBnGen 792700
  Show dependency treegraph
 
Reported: 2012-01-31 14:01 PST by Josh Matthews [:jdm]
Modified: 2012-10-18 13:35 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add privacy information to image requests, and use a separate cache for private requests. (34.43 KB, patch)
2012-03-31 01:59 PDT, Josh Matthews [:jdm]
no flags Details | Diff | Review
Add privacy information to image requests, and use a separate cache for private requests. (33.76 KB, patch)
2012-03-31 02:25 PDT, Josh Matthews [:jdm]
no flags Details | Diff | Review
Add privacy information to image requests, and use a separate cache for private requests. (34.00 KB, patch)
2012-03-31 02:55 PDT, Josh Matthews [:jdm]
no flags Details | Diff | Review
Bug - Tests for imgLoader privacy-respecting changes. (9.34 KB, patch)
2012-04-11 13:40 PDT, Josh Matthews [:jdm]
justin.lebar+bug: feedback+
Details | Diff | Review
Add privacy information to image requests, and use a separate cache for private requests. (36.15 KB, patch)
2012-04-11 13:41 PDT, Josh Matthews [:jdm]
justin.lebar+bug: feedback-
Details | Diff | Review
Add privacy information to image requests, and use a separate cache for private requests. WIP (59.68 KB, patch)
2012-05-10 01:10 PDT, Josh Matthews [:jdm]
no flags Details | Diff | Review
Add privacy information to image requests, and use a separate cache for private requests. (72.51 KB, patch)
2012-05-25 04:29 PDT, Josh Matthews [:jdm]
justin.lebar+bug: feedback+
Details | Diff | Review
Bug - Tests for imgLoader privacy-respecting changes. (4.59 KB, patch)
2012-06-25 21:20 PDT, Josh Matthews [:jdm]
joe: review+
Details | Diff | Review
Add privacy information to image requests, and use a separate cache for private requests. (73.30 KB, patch)
2012-06-25 21:23 PDT, Josh Matthews [:jdm]
joe: review+
Details | Diff | Review
Add privacy information to image requests, and use a separate cache for private requests. (73.90 KB, patch)
2012-08-27 12:41 PDT, Josh Matthews [:jdm]
no flags Details | Diff | Review

Description Josh Matthews [:jdm] 2012-01-31 14:01:41 PST
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] 2012-03-31 01:59:54 PDT
Created attachment 611135 [details] [diff] [review]
Add privacy information to image requests, and use a separate cache for private requests.
Comment 2 Josh Matthews [:jdm] 2012-03-31 02:25:15 PDT
Created attachment 611138 [details] [diff] [review]
Add privacy information to image requests, and use a separate cache for private requests.
Comment 3 Josh Matthews [:jdm] 2012-03-31 02:55:13 PDT
Created attachment 611141 [details] [diff] [review]
Add privacy information to image requests, and use a separate cache for private requests.
Comment 4 Mozilla RelEng Bot 2012-03-31 04:17:33 PDT
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/josh@joshmatthews.net-fb942fcbfa73
Comment 5 Mozilla RelEng Bot 2012-03-31 07:16:53 PDT
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/josh@joshmatthews.net-c4180159869b
Comment 6 Mozilla RelEng Bot 2012-03-31 14:32:45 PDT
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/josh@joshmatthews.net-dd9dfd7aa756
Comment 7 :Ehsan Akhgari (busy, don't ask for review please) 2012-04-04 20:17:12 PDT
Is this ready for review?
Comment 8 Josh Matthews [:jdm] 2012-04-04 20:19:31 PDT
No; I want to write tests first to ensure this is doing what I think it is.
Comment 9 Josh Matthews [:jdm] 2012-04-11 13:40:51 PDT
Created attachment 614149 [details] [diff] [review]
Bug  - Tests for imgLoader privacy-respecting changes.
Comment 10 Josh Matthews [:jdm] 2012-04-11 13:41:07 PDT
Created attachment 614150 [details] [diff] [review]
Add privacy information to image requests, and use a separate cache for private requests.
Comment 11 Josh Matthews [:jdm] 2012-04-11 13:44:27 PDT
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] 2012-04-11 13:47:28 PDT
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] 2012-04-11 13:48:54 PDT
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] 2012-04-12 06:34:45 PDT
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) 2012-04-24 13:37:07 PDT
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) 2012-04-25 11:58:00 PDT
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) 2012-04-25 12:04:55 PDT
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] 2012-05-09 01:46:22 PDT
(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] 2012-05-10 01:10:02 PDT
Created attachment 622650 [details] [diff] [review]
Add privacy information to image requests, and use a separate cache for private requests. WIP
Comment 20 Josh Matthews [:jdm] 2012-05-10 01:19:14 PDT
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 2012-05-10 03:02:02 PDT
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/josh@joshmatthews.net-f9d538b51a57
Comment 22 Josh Matthews [:jdm] 2012-05-10 03:27:20 PDT
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 2012-05-10 03:47:12 PDT
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/josh@joshmatthews.net-406a7a7f69c6
Comment 24 Josh Matthews [:jdm] 2012-05-25 04:29:04 PDT
Created attachment 627178 [details] [diff] [review]
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] 2012-05-25 04:31:21 PDT
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) 2012-05-25 07:48:03 PDT
(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] 2012-06-25 21:20:37 PDT
Created attachment 636589 [details] [diff] [review]
Bug  - Tests for imgLoader privacy-respecting changes.
Comment 28 Josh Matthews [:jdm] 2012-06-25 21:23:36 PDT
Created attachment 636592 [details] [diff] [review]
Add privacy information to image requests, and use a separate cache for private requests.
Comment 29 Joe Drew (not getting mail) 2012-07-16 12:26:04 PDT
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] 2012-08-16 16:11:28 PDT
https://tbpl.mozilla.org/?tree=Try&rev=764abd7b176f
Comment 31 Josh Matthews [:jdm] 2012-08-16 19:05:58 PDT
https://tbpl.mozilla.org/?tree=Try&rev=db869f1ade36
Comment 33 Josh Matthews [:jdm] 2012-08-27 12:31:56 PDT
ttps://hg.mozilla.org/integration/mozilla-inbound/rev/da1ba554adb8
Comment 34 Josh Matthews [:jdm] 2012-08-27 12:41:25 PDT
https://tbpl.mozilla.org/?tree=Try&rev=4d699eb5288d
Comment 35 Josh Matthews [:jdm] 2012-08-27 12:41:47 PDT
Created attachment 655711 [details] [diff] [review]
Add privacy information to image requests, and use a separate cache for private requests.
Comment 36 Josh Matthews [:jdm] 2012-08-28 11:20:11 PDT
https://tbpl.mozilla.org/?tree=Try&rev=6ae02e6d2b9c
Comment 39 Josh Matthews [:jdm] 2012-09-17 06:02:39 PDT
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] 2012-09-17 06:07:28 PDT
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] 2012-10-18 13:35:40 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.