imgLoader uses global PB notifications to clear the image cache

RESOLVED FIXED in mozilla18

Status

()

Core
ImageLib
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jdm, Assigned: jdm)

Tracking

({addon-compat, dev-doc-needed})

Trunk
mozilla18
x86
Mac OS X
addon-compat, dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 8 obsolete attachments)

4.59 KB, patch
Joe Drew (not getting mail)
: review+
Details | Diff | Splinter Review
73.90 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
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.
(Assignee)

Updated

6 years ago
Depends on: 725210
(Assignee)

Comment 1

6 years ago
Created attachment 611135 [details] [diff] [review]
Add privacy information to image requests, and use a separate cache for private requests.
(Assignee)

Comment 2

6 years ago
Created attachment 611138 [details] [diff] [review]
Add privacy information to image requests, and use a separate cache for private requests.
(Assignee)

Updated

6 years ago
Attachment #611135 - Attachment is obsolete: true
(Assignee)

Comment 3

6 years ago
Created attachment 611141 [details] [diff] [review]
Add privacy information to image requests, and use a separate cache for private requests.
(Assignee)

Updated

6 years ago
Attachment #611138 - Attachment is obsolete: true

Comment 4

6 years ago
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

6 years ago
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

5 years ago
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
Is this ready for review?
(Assignee)

Comment 8

5 years ago
No; I want to write tests first to ensure this is doing what I think it is.
(Assignee)

Updated

5 years ago
Assignee: nobody → josh
(Assignee)

Comment 9

5 years ago
Created attachment 614149 [details] [diff] [review]
Bug  - Tests for imgLoader privacy-respecting changes.
(Assignee)

Comment 10

5 years ago
Created attachment 614150 [details] [diff] [review]
Add privacy information to image requests, and use a separate cache for private requests.
(Assignee)

Updated

5 years ago
Attachment #611141 - Attachment is obsolete: true
(Assignee)

Comment 11

5 years ago
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.
Attachment #614149 - Flags: review?(joe)
(Assignee)

Comment 12

5 years ago
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.
Attachment #614150 - Flags: review?(joe)
(Assignee)

Comment 13

5 years ago
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?
(Assignee)

Updated

5 years ago
Depends on: 722845
(Assignee)

Comment 14

5 years ago
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.
(Assignee)

Updated

5 years ago
Attachment #614150 - Flags: feedback?(justin.lebar+bug)
(Assignee)

Updated

5 years ago
Attachment #614149 - Flags: feedback?(justin.lebar+bug)
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
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.
Attachment #614149 - Flags: feedback?(justin.lebar+bug) → feedback-
Attachment #614150 - Flags: feedback?(justin.lebar+bug) → feedback-
Attachment #614149 - Flags: feedback- → feedback?(justin.lebar+bug)
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.
Attachment #614149 - Flags: feedback?(justin.lebar+bug) → feedback+
Attachment #614149 - Flags: review?(joe)
Attachment #614150 - Flags: review?(joe)
(Assignee)

Comment 18

5 years ago
(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.
(Assignee)

Comment 19

5 years ago
Created attachment 622650 [details] [diff] [review]
Add privacy information to image requests, and use a separate cache for private requests. WIP
(Assignee)

Updated

5 years ago
Attachment #622650 - Attachment description: Add privacy information to image requests, and use a separate cache for private requests. → Add privacy information to image requests, and use a separate cache for private requests. WIP
(Assignee)

Updated

5 years ago
Attachment #614150 - Attachment is obsolete: true
(Assignee)

Comment 20

5 years ago
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

5 years ago
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
(Assignee)

Comment 22

5 years ago
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.
Attachment #622650 - Attachment is obsolete: true

Comment 23

5 years ago
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
(Assignee)

Comment 24

5 years ago
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.
(Assignee)

Comment 25

5 years ago
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.
Attachment #627178 - Flags: feedback?(justin.lebar+bug)
(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)|.  :)
Attachment #627178 - Flags: feedback?(justin.lebar+bug) → feedback+
(Assignee)

Comment 27

5 years ago
Created attachment 636589 [details] [diff] [review]
Bug  - Tests for imgLoader privacy-respecting changes.
(Assignee)

Updated

5 years ago
Attachment #614149 - Attachment is obsolete: true
(Assignee)

Comment 28

5 years ago
Created attachment 636592 [details] [diff] [review]
Add privacy information to image requests, and use a separate cache for private requests.
Attachment #636592 - Flags: review?(joe)
(Assignee)

Updated

5 years ago
Attachment #627178 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #636589 - Flags: review?(joe)
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.
Attachment #636592 - Flags: review?(joe) → review+
Attachment #636589 - Flags: review?(joe) → review+
(Assignee)

Comment 30

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=764abd7b176f
(Assignee)

Comment 31

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=db869f1ade36
(Assignee)

Comment 32

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3333e6e37aa
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b4ab96903f2
(Assignee)

Comment 33

5 years ago
ttps://hg.mozilla.org/integration/mozilla-inbound/rev/da1ba554adb8
(Assignee)

Comment 34

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=4d699eb5288d
(Assignee)

Comment 35

5 years ago
Created attachment 655711 [details] [diff] [review]
Add privacy information to image requests, and use a separate cache for private requests.
(Assignee)

Updated

5 years ago
Attachment #636592 - Attachment is obsolete: true
(Assignee)

Comment 36

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=6ae02e6d2b9c
(Assignee)

Comment 37

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/08922991ac7d
https://hg.mozilla.org/integration/mozilla-inbound/rev/bdc68f5ddb38
https://hg.mozilla.org/mozilla-central/rev/08922991ac7d
https://hg.mozilla.org/mozilla-central/rev/bdc68f5ddb38
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Depends on: 789546
Depends on: 789909
(Assignee)

Comment 39

5 years ago
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.
Keywords: addon-compat, dev-doc-needed
(Assignee)

Comment 40

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

Updated

5 years ago
Blocks: 792700

Updated

5 years ago
Depends on: 803150
(Assignee)

Comment 41

5 years ago
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.
You need to log in before you can comment on or make changes to this bug.