Closed Bug 1202085 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox44 --- fixed
firefox45 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

Attachments

(6 files, 5 obsolete files)

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
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.
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.
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..
Assignee: ehsan → nobody
Blocks: ServiceWorkers-postv1
No longer blocks: 1173811
Status: ASSIGNED → NEW
Blocks: ServiceWorkers-v1
No longer blocks: ServiceWorkers-postv1
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
(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: nobody → ehsan
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.
Attachment #8679623 - Flags: review?(seth)
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)
Attachment #8679626 - Flags: review?(josh) → review+
Attachment #8679628 - Flags: review?(josh) → review+
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.
Attachment #8680150 - Flags: review?(seth)
Attachment #8679624 - Attachment is obsolete: true
Attachment #8679624 - Flags: review?(seth)
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)
Attachment #8679627 - Attachment is obsolete: true
Attachment #8679627 - 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+
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)
Attachment #8680151 - Attachment is obsolete: true
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)
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)
Attachment #8680201 - Attachment is obsolete: true
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+
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
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)
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)
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.