(no longer active) Assignee
	Description • 9 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.

	Seth Fowler [:seth] [:s2h]
	Comment 1 • 9 years ago

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.

	(no longer active) Assignee
	Comment 2 • 9 years ago

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

	(no longer active) Assignee
	Comment 3 • 9 years ago

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

	(no longer active) Assignee
	Comment 4 • 9 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.

	(no longer active) Assignee
	Comment 5 • 9 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.  :(

	(no longer active) Assignee
	Comment 7 • 9 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.

	(no longer active) Assignee
	Comment 8 • 9 years ago

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>

	(no longer active) Assignee
	Comment 12 • 9 years ago

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.

	(no longer active) Assignee
	Comment 14 • 9 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...

	(no longer active) Assignee
	Comment 15 • 9 years ago

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

	(no longer active) Assignee
	Comment 16 • 9 years ago

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.

	Seth Fowler [:seth] [:s2h]
	Comment 18 • 9 years ago

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.

	Seth Fowler [:seth] [:s2h]
	Comment 19 • 9 years ago

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.

	Seth Fowler [:seth] [:s2h]
	Comment 20 • 9 years ago

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.

	Seth Fowler [:seth] [:s2h]
	Comment 21 • 9 years ago

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|

	(no longer active) Assignee
	Comment 22 • 9 years ago

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.

	(no longer active) Assignee
	Comment 23 • 9 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?

	(no longer active) Assignee
	Comment 24 • 9 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.

	(no longer active) Assignee
	Comment 25 • 9 years ago

Looks like this worked!

	Seth Fowler [:seth] [:s2h]
	Comment 26 • 9 years ago

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"

	Pulsebot
	Comment 27 • 9 years ago

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

	(no longer active) Assignee
	Comment 28 • 9 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.

	Phil Ringnalda (:philor)
	Comment 29 • 9 years ago
bugherder

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

	Carsten Book [:Tomcat]
	Comment 30 • 9 years ago
bugherder uplift

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

	Ben Kelly [:bkelly, not reviewing]
	Comment 31 • 9 years ago

If this is uplifted to 44 aurora, please also uplift bug 1221279.

	Carsten Book [:Tomcat]
	Comment 32 • 9 years ago

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 33 • 9 years ago

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.

	Carsten Book [:Tomcat]
	Comment 34 • 9 years ago

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)

	Ben Kelly [:bkelly, not reviewing]
	Comment 35 • 9 years ago

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

	Ben Kelly [:bkelly, not reviewing]
	Comment 36 • 9 years ago

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

	Carsten Book [:Tomcat]
	Comment 37 • 9 years ago
bugherder uplift

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

	Carsten Book [:Tomcat]
	Comment 38 • 9 years ago
bugherder uplift

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

	Ian Nartowicz
	Comment 39 • 9 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.

	Ben Kelly [:bkelly, not reviewing]
	Comment 40 • 9 years ago

Ehsan is on holiday.  Seth, do you know the answer to comment 39?

	Seth Fowler [:seth] [:s2h]
	Comment 41 • 9 years ago

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

	Ian Nartowicz
	Comment 42 • 9 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.

	Ben Kelly [:bkelly, not reviewing]
	Comment 43 • 9 years ago

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

	Daniel Glazman (:glazou) (not active in Mozilla any more)
	Comment 44 • 8 years ago

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.