bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Do not store images that have been synthesized by a service worker in the image cache

RESOLVED FIXED in Firefox 44, Firefox OS v2.5

Status

()

Core
ImageLib
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

unspecified
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed, firefox45 fixed, b2g-v2.5 fixed)

Details

Attachments

(6 attachments, 5 obsolete attachments)

9.42 KB, patch
Details | Diff | Splinter Review
2.10 KB, patch
seth
: review+
Details | Diff | Splinter Review
1.43 KB, patch
jdm
: review+
Details | Diff | Splinter Review
8.22 KB, patch
jdm
: review+
Details | Diff | Splinter Review
7.24 KB, patch
seth
: review+
Details | Diff | Splinter Review
5.36 KB, patch
seth
: review+
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
We need to add the URL of the controller of the document to ImageCacheKey so that an imgRequest object does not end up being attached to two imgRequestProxy objects belonging to two documents that are not controlled by the service worker.

This way we can properly propagate the CORS mode from an interception on the service worker side to the img element loading an image.  This is part of the infrastructure that we need to fix bug 1173811.

Updated

3 years ago
Status: NEW → ASSIGNED
This might be more difficult than expected, because the existing imgLoader code expects that we can rematerialize an ImageCacheKey given only an nsIURI or an ImageURL.
(Assignee)

Comment 2

3 years ago
Looks like this is limited to the imgICache and imgILoader methods that take in URIs.
(Assignee)

Comment 3

3 years ago
This doesn't block bug 1173811 any more.  Putting it in the v3 bucket for now..
Assignee: ehsan → nobody
Blocks: 1173500
No longer blocks: 1173811
Status: ASSIGNED → NEW
(Assignee)

Updated

3 years ago
Blocks: 1059784
No longer blocks: 1173500
(Assignee)

Comment 4

3 years ago
We have decided to do something different: don't store images that have been synthesized by a service worker in the image cache.  This should avoid most of the suckiness of this situation for web developers debugging service worker related code for now.
Summary: Do not share image cache entries between controlled and non-controlled documents → Do not store images that have been synthesized by a service worker in the image cache
(Assignee)

Comment 5

3 years ago
(In reply to Ehsan Akhgari (don't ask for review please) from comment #4)
> We have decided to do something different: don't store images that have been
> synthesized by a service worker in the image cache.  This should avoid most
> of the suckiness of this situation for web developers debugging service
> worker related code for now.

Except that this only solves half of the problem, that is, when you visit a controlled document serving image A, and then navigate to an uncontrolled document serving the same image URL.  If you visit the pages in the other order, the non-controlled page puts the image in the image cache, and then in imgLoader::LoadImage() we successfully find the image URL in the image cache and bypass loading it from the network (and hence from the service worker.)

I'm attaching a WIP patch momentarily.  That test case passes if you comment out the <img> tag in register.html.

I have no idea how to fix that without disabling the imagecache completely from all controlled documents.  :(
(Assignee)

Comment 6

3 years ago
Created attachment 8678362 [details] [diff] [review]
WIP for bug 1202085
(Assignee)

Updated

3 years ago
Assignee: nobody → ehsan
(Assignee)

Comment 7

3 years ago
Jeff looked into what Chrome does in their image cache, and they seem to get away with a simple per-doc image cache.  Im going to explore the idea of giving each controlled document its own image cache.  That should fix most long-term issues with the interaction of the image cache and the service worker.
(Assignee)

Comment 9

3 years ago
Created attachment 8679623 [details] [diff] [review]
Part 1: Remove imgICache::RemoveEntry()
Attachment #8679623 - Flags: review?(seth)
(Assignee)

Comment 10

3 years ago
Created attachment 8679624 [details] [diff] [review]
Part 2: Add an optional document argument to imgICache::FindEntryProperties()
Attachment #8679624 - Flags: review?(seth)
(Assignee)

Comment 11

3 years ago
Created attachment 8679626 [details] [diff] [review]
Part 3: Relax the assertion in ServiceWorkerManager::IsControlled to only happen when we think the document is controlled
Attachment #8679626 - Flags: review?(josh)
(Assignee)

Comment 12

3 years ago
Created attachment 8679627 [details] [diff] [review]
Part 4: Add an ID for controlled document to the image cache key

This ID will be null for non-controlled documents and also for image cache
entries for which a document is not available, and it will be the numerical
value of the document pointer for controlled documents.

This effectively makes sure that a controlled document doesn't share its
image cache entries with anything else.
Attachment #8679627 - Flags: review?(seth)
(Assignee)

Comment 13

3 years ago
Created attachment 8679628 [details] [diff] [review]
Part 5: Add an automated test for the interaction of image cache with controlled documents
Attachment #8679628 - Flags: review?(josh)

Updated

3 years ago
Attachment #8679626 - Flags: review?(josh) → review+

Updated

3 years ago
Attachment #8679628 - Flags: review?(josh) → review+
(Assignee)

Comment 14

3 years ago
Another thing left to do is to clear the cache entries belonging to a document when it goes away.  I'll submit a patch for that tomorrow...
(Assignee)

Comment 15

3 years ago
Created attachment 8680150 [details] [diff] [review]
Part 2: Add an optional document argument to imgICache::FindEntryProperties()

nsIDocument isn't an XPIDL interface so it can't be used like this.  I need to use nsIDOMDocument instead.
Attachment #8680150 - Flags: review?(seth)
(Assignee)

Updated

3 years ago
Attachment #8679624 - Attachment is obsolete: true
Attachment #8679624 - Flags: review?(seth)
(Assignee)

Comment 16

3 years ago
Created attachment 8680151 [details] [diff] [review]
Part 4: Add an ID for controlled document to the image cache key

This ID will be null for non-controlled documents and also for image cache
entries for which a document is not available, and it will be the numerical
value of the document pointer for controlled documents.

This effectively makes sure that a controlled document doesn't share its
image cache entries with anything else.
Attachment #8680151 - Flags: review?(seth)
(Assignee)

Updated

3 years ago
Attachment #8679627 - Attachment is obsolete: true
Attachment #8679627 - Flags: review?(seth)
(Assignee)

Comment 17

3 years ago
Created attachment 8680201 [details] [diff] [review]
Part 6: Clear the entries in the image cache belonging to a controlled document when it gets destroyed
Attachment #8680201 - Flags: review?(seth)
Comment on attachment 8679623 [details] [diff] [review]
Part 1: Remove imgICache::RemoveEntry()

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

We discussed this on IRC; should be OK.
Attachment #8679623 - Flags: review?(seth) → review+
Comment on attachment 8680150 [details] [diff] [review]
Part 2: Add an optional document argument to imgICache::FindEntryProperties()

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

Looks good.

::: image/imgLoader.cpp
@@ +1334,5 @@
>    }
>  }
>  
>  NS_IMETHODIMP
> +imgLoader::FindEntryProperties(nsIURI* uri, nsIDOMDocument* doc,

Nit: please just put each argument on its own line.

Are you going to actually use |doc|? I'm guessing you do so in a followup patch.
Attachment #8680150 - Flags: review?(seth) → review+
Comment on attachment 8680151 [details] [diff] [review]
Part 4: Add an ID for controlled document to the image cache key

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

Looks good, but needs a couple of tweaks:

::: image/ImageCacheKey.cpp
@@ +132,3 @@
>    }
>  
>    // For non-blob URIs, we hash the URI spec.

Please include the controlled document in the hash. (Both the blob and non-blob versions.)

::: image/ImageCacheKey.h
@@ +48,5 @@
>  
>  private:
>    static uint32_t ComputeHash(ImageURL* aURI,
>                                const Maybe<uint64_t>& aBlobSerial);
> +  static void* IsDocumentControlled(nsIDOMDocument* aDocument);

This needs to be renamed. |IsDocumentControlled()| sounds like it returns a bool.
Attachment #8680151 - Flags: review?(seth)
Comment on attachment 8680201 [details] [diff] [review]
Part 6: Clear the entries in the image cache belonging to a controlled document when it gets destroyed

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

Looks good!

::: image/ImageCacheKey.h
@@ +45,5 @@
>  
>    /// Is this cache entry for a chrome image?
>    bool IsChrome() const { return mIsChrome; }
>  
> +  void* ControlledDocument() const { return mControlledDocument; }

Nit: A doxygen comment would be nice. (/// is fine.)

::: image/imgICache.idl
@@ +58,5 @@
> +   * Clear the image cache for a document.  Controlled documents are responsible
> +   * to do this manually when they get destroyed.
> +   */
> +  [noscript]
> +  void clearCacheForControlledDocument(in nsIDocument doc);

Just mark it |notxpcom| so you don't need to use NS_IMETHODIMP.

::: image/imgLoader.cpp
@@ +1360,5 @@
>    return NS_OK;
>  }
>  
> +NS_IMETHODIMP
> +imgLoader::ClearCacheForControlledDocument(nsIDocument *aDoc)

Nit: |nsIDocument* aDoc|
Attachment #8680201 - Flags: review?(seth) → review+
(Assignee)

Comment 22

3 years ago
Created attachment 8680265 [details] [diff] [review]
Part 4: Add an ID for controlled document to the image cache key

This ID will be null for non-controlled documents and also for image cache
entries for which a document is not available, and it will be the numerical
value of the document pointer for controlled documents.

This effectively makes sure that a controlled document doesn't share its
image cache entries with anything else.
Attachment #8680265 - Flags: review?(seth)
(Assignee)

Updated

3 years ago
Attachment #8680151 - Attachment is obsolete: true
(Assignee)

Comment 23

3 years ago
On the try server, I'm getting shutdown assertions like this <http://archive.mozilla.org/pub/firefox/try-builds/eakhgari@mozilla.com-edd22b81c68521cd29cbf7811e87edcf3d82d369/try-linux64-debug/try_ubuntu64_vm-debug_test-mochitest-1-bm113-tests1-linux64-build795.txt.gz> (which turn into opt crashes like <http://archive.mozilla.org/pub/firefox/try-builds/eakhgari@mozilla.com-edd22b81c68521cd29cbf7811e87edcf3d82d369/try-linux64/try_ubuntu64_vm_test-mochitest-4-bm116-tests1-linux64-build1040.txt.gz>.  The assertion suggests that mUncachedImagesMutex is somehow invalid...  And note that this happens when the test suite is shutting down, so it's tough to say what causes this.

Do you have any ideas, Seth?
Flags: needinfo?(seth)
(Assignee)

Comment 24

3 years ago
Comment on attachment 8680265 [details] [diff] [review]
Part 4: Add an ID for controlled document to the image cache key

(In reply to Ehsan Akhgari (don't ask for review please) [Away Nov 3-19] from comment #23)
> Do you have any ideas, Seth?

Hmm, I may have a fix...  I think the way I remove the cache entry is wrong.  Testing on try right now.
Attachment #8680265 - Attachment is obsolete: true
Attachment #8680265 - Flags: review?(seth)
(Assignee)

Comment 25

3 years ago
Created attachment 8680813 [details] [diff] [review]
Part 6: Clear the entries in the image cache belonging to a controlled document when it gets destroyed

Looks like this worked!
Attachment #8680813 - Flags: review?(seth)
(Assignee)

Updated

3 years ago
Attachment #8680201 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Flags: needinfo?(seth)
Comment on attachment 8680813 [details] [diff] [review]
Part 6: Clear the entries in the image cache belonging to a controlled document when it gets destroyed

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

Looks good!

::: image/imgICache.idl
@@ +55,5 @@
>    void respectPrivacyNotifications();
> +
> +  /**
> +   * Clear the image cache for a document.  Controlled documents are responsible
> +   * to do this manually when they get destroyed.

Nit: "to do" -> "for doing"
Attachment #8680813 - Flags: review?(seth) → review+
(Assignee)

Comment 28

3 years ago
Comment on attachment 8679623 [details] [diff] [review]
Part 1: Remove imgICache::RemoveEntry()

Requesting approval for all patches in the series.


Approval Request Comment
[Feature/regressing bug #]: This is needed for service workers on Aurora.
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]: Has a test.
[Risks and why]:  Should have landed before the cut-off.  But the risk should not be great to non-service worker related stuff.
[String/UUID change made/needed]: No string changes, some uuid changes.
Attachment #8679623 - Flags: approval-mozilla-aurora?
If this is uplifted to 44 aurora, please also uplift bug 1221279.
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
status-b2g-v2.5: fixed → ---

Updated

3 years ago
status-firefox44: --- → affected
Comment on attachment 8679623 [details] [diff] [review]
Part 1: Remove imgICache::RemoveEntry()

I don't think there is a choice here really, this seems to be left over from Service worker feature planned for Nightly44 into Aurora44. Let's land it now.
Attachment #8679623 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
part 5 failed to apply to aurora 

grafting 312310:e221700129d5 "Bug 1202085 - Part 5: Add an automated test for the interaction of image cache with controlled documents; r=jdm"
merging dom/workers/test/serviceworkers/mochitest.ini
warning: conflicts during merge.
merging dom/workers/test/serviceworkers/mochitest.ini incomplete! (edit conflicts, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)
Flags: needinfo?(rkothari)
I'll take a look at the merge conflict today.
Flags: needinfo?(rkothari) → needinfo?(bkelly)

Comment 39

3 years ago
Was it really intended to delete the removeEntry() method from imgICache (and imgITools)?  I haven't seen the IRC discussions, but this is a long-standing feature which seems to be necessary even after Cache Storage arrives.
Ehsan is on holiday.  Seth, do you know the answer to comment 39?
Flags: needinfo?(seth)
(In reply to Ian Nartowicz from comment #39)
> Was it really intended to delete the removeEntry() method from imgICache
> (and imgITools)?  I haven't seen the IRC discussions, but this is a
> long-standing feature which seems to be necessary even after Cache Storage
> arrives.

It could not remain with the API that it had previously; things are just not simple enough that a simple URI suffices anymore.

It sounds like you have a concrete use case in mind that cannot be satisfied without removeEntry(); what is it? I'm not aware of such a use case.
Flags: needinfo?(seth) → needinfo?(mozilla)

Comment 42

3 years ago
My specific use case is to display a button icon from a URL.  The URL doesn't change but the underlying image might.  In that case, the only solution appeared to be directly evicting it from the image cache with removeEntry().  I was prompted to investigate this by another user with essentially the same situation whose addon failed in recent nightlies.  If the new caching API offers a solution to this situation, I haven't been able to fathom it.
Flags: needinfo?(mozilla)
Ian, do either of these workarounds handle your situation?

1) Add a cache-busting random number in the URL fragment or query.
2) Read the image into a blob, use URL.createObjectURL(), and set the resulting blob URL as img.src?  (Don't forget to call URL.revokeObjectURL() after img.onload fires.)
Flags: needinfo?(mozilla)
Flags: needinfo?(mozilla)
FWIW, this horked a major feature of BlueGriffon and blocked 2.0 release: reload of
replaced objects when modified outside of BlueGriffon is relying on |imgICache.removeEntry()|.

I had to revert the part1 change in my code because the only remaining option from JS was
to clear the entire cache for the edited document, a bit overkill. Not a problem to keep
that change in my tree but still worth mentioning it here, IMO.

I guess I'm with Ian here: I'd love to see that rather cheap feature come back to imgICache.
Thanks.
You need to log in before you can comment on or make changes to this bug.