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.

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.

Looks like this is limited to the imgICache and imgILoader methods that take in URIs.

This doesn't block bug 1173811 any more. Putting it in the v3 bucket for now..

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.

(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. :(

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.

Relevant code in Blink: <https://github.com/ChromiumWebApps/blink/blob/master/Source/core/fetch/ResourceFetcher.h#L64> <https://github.com/ChromiumWebApps/blink/blob/master/Source/core/fetch/ResourceFetcher.cpp#L620>

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.

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...

nsIDocument isn't an XPIDL interface so it can't be used like this. I need to use nsIDOMDocument instead.

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.

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.

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.

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.

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|

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.

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?

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.

Looks like this worked!

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"

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c5e6d68cc46 https://hg.mozilla.org/integration/mozilla-inbound/rev/0095ad1a7b09 https://hg.mozilla.org/integration/mozilla-inbound/rev/b9a0f07e974a https://hg.mozilla.org/integration/mozilla-inbound/rev/afc27ae3cc45 https://hg.mozilla.org/integration/mozilla-inbound/rev/e221700129d5 https://hg.mozilla.org/integration/mozilla-inbound/rev/e08508d502c8

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.

https://hg.mozilla.org/mozilla-central/rev/9c5e6d68cc46 https://hg.mozilla.org/mozilla-central/rev/0095ad1a7b09 https://hg.mozilla.org/mozilla-central/rev/b9a0f07e974a https://hg.mozilla.org/mozilla-central/rev/afc27ae3cc45 https://hg.mozilla.org/mozilla-central/rev/e221700129d5 https://hg.mozilla.org/mozilla-central/rev/e08508d502c8

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/9c5e6d68cc46 https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/0095ad1a7b09 https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/b9a0f07e974a https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/afc27ae3cc45 https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/e221700129d5 https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/e08508d502c8

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

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.

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)

I'll take a look at the merge conflict today.

remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/8b609ab4b121 remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/a00212c1f5ca remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/8802cf4e9ecf remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/5289a341cf53 remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/4f2fb4f1bd94 remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/e42e1ac395ae

https://hg.mozilla.org/releases/mozilla-aurora/rev/8b609ab4b121 https://hg.mozilla.org/releases/mozilla-aurora/rev/a00212c1f5ca https://hg.mozilla.org/releases/mozilla-aurora/rev/8802cf4e9ecf https://hg.mozilla.org/releases/mozilla-aurora/rev/5289a341cf53 https://hg.mozilla.org/releases/mozilla-aurora/rev/4f2fb4f1bd94 https://hg.mozilla.org/releases/mozilla-aurora/rev/e42e1ac395ae

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/8b609ab4b121 https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/a00212c1f5ca https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/8802cf4e9ecf https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/5289a341cf53 https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/4f2fb4f1bd94 https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/e42e1ac395ae

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?

(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.

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.

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.)

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.