Closed Bug 792700 Opened 7 years ago Closed 7 years ago

Convert all JS users of imgICache service to use privacy-aware imgITools API instead (SeaMonkey part).

Categories

(SeaMonkey :: General, defect)

defect
Not set

Tracking

(seamonkey2.15 fixed, seamonkey2.16 fixed)

RESOLVED FIXED
seamonkey2.16
Tracking Status
seamonkey2.15 --- fixed
seamonkey2.16 --- fixed

People

(Reporter: philip.chee, Assigned: philip.chee)

References

Details

Attachments

(1 file, 3 obsolete files)

From Bug 789546 Comment 0:
====================================================
STR:
1. View a data:image URI such as:

data:image/gif;base64,R0lGODlhEAAOALMAAOazToeHh0tLS/7LZv/0jvb29t/f3//Ub//ge8WSLf/rhf/3kdbW1mxsbP//mf///yH5BAAAAAAALAAAAAAQAA4AAARe8L1Ekyky67QZ1hLnjM5UUde0ECwLJoExKcppV0aCcGCmTIHEIUEqjgaORCMxIC6e0CcguWw6aFjsVMkkIr7g77ZKPJjPZqIyd7sJAgVGoEGv2xsBxqNgYPj/gAwXEQA7

2. Right-click on the tiny image and `Save Image As`

AR:
Starting with Nightly 18.0a1 (2012-09-01), Firefox saves a file called "index" (with no ".gif" filename extension).

ER:
Before Nightly 2012-09-01, Firefox saved a file called "index.gif" with a proper filename extension. (The base filename is "index" because the data:image does not have its own name or title).
====================================================

As far as I can tell we don't have this bug either with or without this patch. But the signature of saveImageURL() has changed so we need to change the callers anyway.
Attached patch Patch v1.0 Proposed fix. (obsolete) — Splinter Review
> +        var imageCache = Components.classes["@mozilla.org/image/tools;1"]
(NB: This is from Bug 576621 - clearing cache does NOT clear cached images)

> +                                   .getService(Components.interfaces.imgITools)
> +                                   .getImgCacheForDocument(null);
The IDL says that "@param doc A document. Must not be null." but according to MXR we pass null to it all over the place?

http://mxr.mozilla.org/comm-central/source/mozilla/image/public/imgITools.idl#93

> +        try {
> +          imageCache.clearCache(false); // true=chrome, false=content
> +        } catch(ex) {}
Can this really throw?

The relevant test passes with or without this patch so it doesn't tell us anything:
TEST_PATH=toolkit/content/tests/browser/browser_default_image_filename.js  pymake -C ../objdir-sm/ mochitest-browser-chrome
Attachment #662862 - Flags: review?(neil)
(In reply to Philip Chee from comment #1)
> > +                                   .getService(Components.interfaces.imgITools)
> > +                                   .getImgCacheForDocument(null);
> The IDL says that "@param doc A document. Must not be null." but according
> to MXR we pass null to it all over the place?
The code uses it to decide whether to hand you the global image cache or the private browsing image cache. There's an early return when you pass null so that you always get the global image cache.

> > +        try {
> > +          imageCache.clearCache(false); // true=chrome, false=content
> > +        } catch(ex) {}
> Can this really throw?
In theory it can, but it seems it would be really unusual.
I filed bug 792821 about improving the documentation for getImgCacheForDocument.
Comment on attachment 662862 [details] [diff] [review]
Patch v1.0 Proposed fix.

>       clear: function() {
>         var cacheService = Components.classes["@mozilla.org/network/cache-service;1"]
>                                      .getService(Components.interfaces.nsICacheService);
>         cacheService.evictEntries(Components.interfaces.nsICache.STORE_ANYWHERE);
>+
>+        var imageCache = Components.classes["@mozilla.org/image/tools;1"]
>+                                   .getService(Components.interfaces.imgITools)
>+                                   .getImgCacheForDocument(null);
>+        try {
>+          imageCache.clearCache(false); // true=chrome, false=content
>+        } catch(ex) {}
The caller wraps this in a try/catch, so putting the try/catch on the last item is unhelpful. On the other hand, if there was an error clearing the disk or memory cache, then the image cache wouldn't get cleared...
> The caller wraps this in a try/catch, so putting the try/catch on the last item
> is unhelpful. On the other hand, if there was an error clearing the disk or
> memory cache, then the image cache wouldn't get cleared...

Changed to try/catch everything but the last task.
Attachment #662862 - Attachment is obsolete: true
Attachment #662862 - Flags: review?(neil)
Attachment #664096 - Flags: review?(neil)
Blocks: 799529
Sigh. The try catch block hid an error caused by my typo. Remind me to remove the logStringMessage()s before checkin.
Attachment #664096 - Attachment is obsolete: true
Attachment #664096 - Flags: review?(neil)
Attachment #671408 - Flags: review?(neil)
Comment on attachment 671408 [details] [diff] [review]
Patch v1.2 Rearrange try/catch block(s) with fewer typos.

>+    var doc =  this.target.ownerDocument;
Nit: doubled space after =

>+        // use try/catch for each task so we clear as much as possible.
Need to copy (not move!) the offlineApps version of the comment.

>+        var imageCache = Components.classes["@mozilla.org/image/tools;1"]
>+                                   .getService(Components.interfaces.imgITools)
>+                                   .getImgCacheForDocument(null);
>+        try {
>+          imageCache.clearCache(false); // true=chrome, false=content.
>+        } catch(ex) {
>+          cs.logStringMessage(ex);
>+        }
Will you be getting rid of this try/catch again?

>-        // use try/catch for everything but the last task so we clear as much as possible
Don't delete this!
Comment on attachment 664096 [details] [diff] [review]
Patch v1.1 Rearrange try/catch block(s).

>+        // use try/catch for everything but the last task so we clear as much as possible.
I see this version did at least have the correct comment here ;-)

>+        Components.classes["@mozilla.org/image/tools;1"]
>+                  .getService(Components.interfaces.imgITools)
>+                  .getImgCacheForDocument(null)
>+                  .imageCache.clearCache(false); // true=chrome, false=content
Ah yes, the extra .imageCache is bogus, amirite?
Comment on attachment 671408 [details] [diff] [review]
Patch v1.2 Rearrange try/catch block(s) with fewer typos.

I'm going to r+ the previous patch because it was less wrong, but I'm not going to fiddle around with the patch flags because you'll need a new one anyway.
Attachment #671408 - Flags: review?(neil) → review-
Attachment #664096 - Flags: review+
>>+    var doc =  this.target.ownerDocument;
> Nit: doubled space after =
Fixed.

>>+        // use try/catch for each task so we clear as much as possible.
> Need to copy (not move!) the offlineApps version of the comment.
Fixed.

> Will you be getting rid of this try/catch again?
Fixed.
Attachment #671408 - Attachment is obsolete: true
Attachment #673253 - Flags: review?(neil)
(In reply to comment #7)
> >-        // use try/catch for everything but the last task so we clear as much as possible
> Don't delete this!
Ah, I see the code no longer follows the comment. Sigh...
Comment on attachment 673253 [details] [diff] [review]
Patch v1.3 address review comments. [check-in comment 13]

>+        // use try/catch for everything but the last task so we clear as much as possible.
Nit: no trailing . (3x)

>+        try {
>+          cacheService.evictEntries(Components.interfaces.nsICache.STORE_ANYWHERE);
>+        } catch(ex) {}
Nit: blank line after here please

>+        Components.classes["@mozilla.org/image/tools;1"]
>+                   .getService(Components.interfaces.imgITools)
>+                   .getImgCacheForDocument(null)
>+                   .clearCache(false); // true=chrome, false=content.
Nit: wrong indentation
Attachment #673253 - Flags: review?(neil) → review+
Comment on attachment 673253 [details] [diff] [review]
Patch v1.3 address review comments. [check-in comment 13]

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/dee6670c667f

>>+        // use try/catch for everything but the last task so we clear as much as possible.
> Nit: no trailing . (3x)

Fixed.

>>+        try {
>>+          cacheService.evictEntries(Components.interfaces.nsICache.STORE_ANYWHERE);
>>+        } catch(ex) {}
> Nit: blank line after here please

Fixed.

>>+        Components.classes["@mozilla.org/image/tools;1"]
>>+                   .getService(Components.interfaces.imgITools)
>>+                   .getImgCacheForDocument(null)
>>+                   .clearCache(false); // true=chrome, false=content.
> Nit: wrong indentation

Fixed.
> Depends on: 722861 789546 
Base bugs landed on Mozilla18/Firefox18 which corresponds to SeaMonkey15 (aurora).
Once this bakes on trunk for a week or two. I'll ask for approval-aurora.
Target Milestone: --- → seamonkey2.16
Firefox extensions that generate screenshots aren't working on Nightly or Aurora. Bug 802472

Mr. Chee kindly made a posting to a thread I started in the Firefox forum about this fact. http://forums.mozillazine.org/viewtopic.php?f=23&t=2576141

I have a couple of observations here; please forgive my possibly mixing issues.

1. The AR described in comment 0 above isn't what I observed today in Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:19.0) Gecko/19.0 Firefox/19.0 ID:20121020030554 CSet: ff4af83233dc

2. The broken extensions somehow appear related to this -> http://forums.mozillazine.org/viewtopic.php?f=48&t=73324&start=505

3. So does this -> http://forums.mozillazine.org/viewtopic.php?f=23&t=2570969

4. I guess this bug (792700) is also somehow involved. 

I assume the ability to use the extended screenshot functionality that several extensions offer is useful to many people (the add-ons Screenshot, Awesome Screenshot Plus, Fireshot, and Screenshot Pimp have a total of roughly half a million users). 

Perhaps I'm getting ahead of the game, but if all these people find the function inexplicably broken in a future release version with no clear explanation, I don't think they'll be happy. (I'd contend the fact that "not all JS users of imgICache service have converted to the use of privacy-aware imgITools API" won't be very helpful to most end-users.)

Again, please excuse me if I've mixed apples and oranges.
Comment on attachment 673253 [details] [diff] [review]
Patch v1.3 address review comments. [check-in comment 13]

> Base bugs landed on Mozilla18/Firefox18 which corresponds to SeaMonkey15
> (aurora). Once this bakes on trunk for a week or two. I'll ask for
> approval-aurora.
Might as well do it now before I forget.

[Approval Request Comment]
Regression caused by (bug #): Bug 789546
User impact if declined: Users may not be able to save images from the content context menu. See 799529.
Testing completed (on m-c, etc.): tested on c-c.
Risk to taking this patch (and alternatives if risky): Low or minimal. bustage fix.
String changes made by this patch: None.
Attachment #673253 - Attachment description: Patch v1.3 address review comments. → Patch v1.3 address review comments. [check-in comment 13]
Attachment #673253 - Flags: approval-comm-aurora?
Attachment #673253 - Flags: approval-comm-aurora? → approval-comm-aurora+
Pushed to comm-aurora:
http://hg.mozilla.org/releases/comm-aurora/rev/288e1cf33afc
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.