1625786 - Incorrect imgElement.currentSrc in WhatsApp Web stickers

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[tustamido]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:72.0) Gecko/20100101 Firefox/72.0

Steps to reproduce:

• Open a WhatsApp Web chat and find a sticker (special 125×125 image). You can also differentiate these from normal images knowing that hovering stickers doesn't change the cursor to pointer, unlike normal images.

• Right-click it to display the standalone image, either by "View image" or by "Copy Image Address" and pasting it in urlbar.

Actual results:

The image is not displayed, instead you get an error message. That's because Firefox sets an invalid currentSrc attribute to the img element and this is the value you get when you click "View image" or "Copy image address".

Expected results:

Image should be displayed, which means that imgElement.currentSrc should be the same as imgElement.src. If you inspect the image on WhatsApp Web, copy its src and open it, then the image will load correctly.

The thing is: Firefox seems to be generating an invalid currentSrc in this case.

This doesn't happen in Blink browsers, so it seems to be a Firefox bug.

Some notes:

• WhatsApp Web media URLs are always blob:https://web.whatsapp.com/GUID
• This bug is not reproducible with normal images attached on chat, just stickers.

Comment 1

[Clara Guerrero ( Need Info Brindusa Tot Please)]

Hi,

I am trying to replicate the issue on windows 10 pro

On the following versions:

stickers:
Release 74.0 (64-bit) "View image" and "Copy Image Address" and pasting it in urlbar won't work
Beta 75.0b11 (64-bit) "View image" and "Copy Image Address" and pasting it in urlbar won't work
Firefox Nightly 76.0a1 (2020-03-31) (64-bit) "View image" and "Copy Image Address" and pasting it in urlbar won't work (This is just for sticker situation)

normal image:
Release 74.0 (64-bit) "View image" works and "Copy Image Address" and pasting it in urlbar works (Just for release, the bug is not reproducible with normal images attached on chat. But for beta and nightly I am not able to see either stickers nor normal images when copying image location/choosing view image.)
Beta 75.0b11 (64-bit) "View image" ¨works but "Copy Image Address" and pasting it in urlbar doesn't work
Firefox Nightly 76.0a1 (2020-03-31) (64-bit) "View image" works but "Copy Image Address" and pasting it in urlbar doesn't work (This is just for normal image situation)

If I inspect the image on WhatsApp Web, copy its src and open it, as suggested in your report, then the image won't load either.

I followed the same steps on chrome and it's working fine either copying image location, or selecting "view image".

I will move this over to a component so developers can take a look over it. If is not the correct component please feel free to change it to an appropriate one.

Thanks for the report.

Best regards, Clara.

Comment 2

[tustamido]

You're right, "Copy Image Address" stopped working in mozilla-central 74 even for normal images. Using mozregression, it seems to have been caused by bug 1597475.

This is not exactly what I reported initially, since I thought it was all fine with normal images (and it was, as I tested with DevEd 72), but probably it's related.

Comment 3

[tustamido]

Just to be clear, what regressed by bug 1597475 was the ability to open WhatsApp's normal images in a new tab. But for these images currentSrc = src.

This bug here is about WhatsApp's stickers having currentSrc != src, the src value being the correct one. This bug is reproducible even before bug 1597475 and wasn't affected by it.

I think these are two different bugs, maybe someone should fill the other.

Comment 4

[tustamido]

If you leftclick "View image" over a sticker, browser console logs:

Error: Load of blob:https://web.whatsapp.com/877e3461-316a-45da-bc63-087b712e0faf from https://web.whatsapp.com/ denied. BrowserUtils.jsm:149:13
urlSecurityCheck resource://gre/modules/BrowserUtils.jsm:149
urlSecurityCheck chrome://global/content/contentAreaUtils.js:35
viewMedia chrome://browser/content/nsContextMenu.js:1270
oncommand chrome://browser/content/browser.xhtml:1

blob:https://web.whatsapp.com/877e3461-316a-45da-bc63-087b712e0faf is the invalid currentSrc that for some reason Firefox is generating (this is the bug). If currentSrc had the value of src attribute, I believe that there would be no error and the image would be displayed as it should when I clicked "View image".

Comment 5

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to tustamido from comment #3)

Just to be clear, what regressed by bug 1597475 was the ability to open WhatsApp's normal images in a new tab. But for these images currentSrc = src.

This bug here is about WhatsApp's stickers having currentSrc != src, the src value being the correct one. This bug is reproducible even before bug 1597475 and wasn't affected by it.

I think these are two different bugs, maybe someone should fill the other.

Do you have different thoughts here, Edgar? Thanks.

Comment 6

[Mike Taylor [:miketaylr]]

Should we consider backing out Bug 1597475? Or do we think the fix is simpler?

Comment 7

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Mike Taylor [:miketaylr] from comment #6)

Should we consider backing out Bug 1597475? Or do we think the fix is simpler?

Also NI the author and reviewer of bug 1597475, thanks!

Comment 8

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

I'm not sure about how bug 1597475 (change in nsDocumentViewer::GetContentSize()) effects to this bug, are there any small test case or other test cases?

I don't use WhatsApp and my friends also don't use it. I don't have a way to reproduce this...

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Are we sure about that regression range? nsDocumentViewer::SizeToContent is used only to implement shrink-to-fit windows, I'd be very confused if that was the culprit.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Can you clarify how you got to that regression range? I can reproduce the issue even with mozregression --launch 72, which doesn't contain bug 1597475.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

I'll take a look because it is odd that .src and .currentSrc disagree here. I want to check where that .currentSrc comes from.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

But pretty sure it's not a regression from that bug.

Comment 13

[tustamido]

Maybe some of you missed my comment #3. This bug here is not a regression, at least not from bug 1597475.

Unintentionally, my STR led to Clara Guerrero (comment #1) to discover another similar bug. This other one that seems to be regressed by bug 1597475.

So now we (me, at least) know two bugs:

This bug here: whatsappSticker.src != whatsappSticker.currentSrc. The correct value is src, the value in currentSrc is invalid (this is the bug). "Copy Image Location" gets the currentSrc, then if you paste it in urlbar, an error will appear. But if you inspect the element, manually copy the src of the img element and try to load this url, then the image will be displayed correctly.

Another bug someone should fill: after bug 1597475, even normal images in WhatsApp Web with img.src == img.currentSrc, if you "Copy Image Location" and paste it in urlbar, the image will not load.

Sorry if I'm not being clear enough, english is not my native language.

Comment 14

[Emilio Cobos Álvarez (:emilio)]

(In reply to tustamido from comment #13)

Maybe some of you missed my comment #3. This bug here is not a regression, at least not from bug 1597475.

Ah, indeed I had missed that, thanks.

Unintentionally, my STR led to Clara Guerrero (comment #1) to discover another similar bug. This other one that seems to be regressed by bug 1597475.

That makes no sense. I re-run mozregression with the STR from comment 1 and I got something that looks much more plausible:

https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=398b5e6b2dab72d5be60bc81b0fda2d2c27bfb25&tochange=b9740bd7bf8f1a0656c09a88fe6d7ee00586ee60

It seems bug 1626573 is kinda similar and is already on file. Baku, Subhamoy, is the "open a new tab, paste blob URI" case also handled here? To be clear, the STR are "Copy Image Location" from an image in whatsapp (from a blob URI), and paste it in urlbar. I suspect it might be different because the navigation is triggered by typing out the uri, so let me know if you want a separate bug on file for that.

This bug here: whatsappSticker.src != whatsappSticker.currentSrc. The correct value is src, the value in currentSrc is invalid (this is the bug). "Copy Image Location" gets the currentSrc, then if you paste it in urlbar, an error will appear. But if you inspect the element, manually copy the src of the img element and try to load this url, then the image will be displayed correctly.

Alright, with that situation clarified, I'll dig into this.

Sorry if I'm not being clear enough, english is not my native language.

Np! (neither mine :)). Thank you so much :)

Comment 15

[Andrea Marchesini [:baku]]

It seems bug 1626573 is kinda similar and is already on file. Baku, Subhamoy, is the "open a new tab, paste blob URI" case also handled here? To be clear, the STR are "Copy Image Location" from an image in whatsapp (from a blob URI), and paste it in urlbar. I suspect it might be different because the navigation is triggered by typing out the uri, so let me know if you want a separate bug on file for that.

What you are describing here in a follow up of bug 1626573. To know more in detail what the problem is about, see bug 1626573 comment 5.
In that bug, we want to fix the opening of a blobURL link from the context menu, and window.open() + noopener.

There is an additional problem to mention: blobURLs are strongly connected with their contexts: if a blobURL is created in a worker when the worker is terminated, the blobURL is released; if a blobURL is created by a document when the document is dismissed, the blobURL goes away.
Because of this, even if we fix the issue, a similar bug can happen using the following STR:

1. in tab A open a page that creates a blob URL
2. copy that blob URL
3. close the tab A
4. paste the blob URL in the urlbar and navigate.

Personally, I think blobURLs should be available only in the agent cluster of their context and, copy&paste, or "open link in a new tab/window", should not be allowed. Anne, do you think we can ask for this change at a spec level?

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Attachment: Reduced test-case.

So the issue goes as follows:

• This is a general issue of trying to "view image" on a revoked blob URI.

• We have an optimization to merge cached entries for different blob URIs that share the same underlying blob (bug 1154974).

• Whatsapp seems to be hitting that optimization, but the first URI that they use gets revoked. The second doesn't though. So we end up with a cached image whose mURI is the revoked uri, but the src attribute is the non-revoked one.

Questions:

• It's not clear to me that this caching is per spec... Can you cache an image request that doesn't share the same URI? We don't even share the image for the same URI with different refs... (see bug 1169680).

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Attachment: More self-descriptive test-case.

Timothy, see above, do you know if this caching is per spec? It seems a bit odd to share images with different URIs but...

Comment 18

[Timothy Nikkel (:tnikkel)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #17)

Created attachment 9140552 [details]
More self-descriptive test-case.

Timothy, see above, do you know if this caching is per spec? It seems a bit odd to share images with different URIs but...

I'm not aware of any spec that talks about caching of images that also mentions blob uris, so the behaviour would be determined by specs about blobs I would think. And I'm not very familiar with blob specs.

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1625786 - Fix HTMLImageElement.currentSrc when we share the underlying blob image data. r=tnikkel

This fixes the web-observable bits, but still not the context menu. Patch
incoming for that.

Comment 20

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1625786 - Make context menu for images properly check for revoked blob URIs. r=Gijs

This fixes the user-facing issue.

Depends on D70936

Comment 21

[:Gijs (he/him)]

(In reply to Andrea Marchesini [:baku] from comment #15)

Because of this, even if we fix the issue, a similar bug can happen using the following STR:

1. in tab A open a page that creates a blob URL
2. copy that blob URL
3. close the tab A
4. paste the blob URL in the urlbar and navigate.

Personally, I think blobURLs should be available only in the agent cluster of their context and, copy&paste, or "open link in a new tab/window", should not be allowed. Anne, do you think we can ask for this change at a spec level?

I think this would be user-unfriendly. My understanding from the other bug is that we already have patches to make the open-in-a-new-tab/window work, and the case in this bug about revoked blob URIs is a bit of a special one. The fact that blob URIs stop working once the original tab closes seems more acceptable, and less likely to cause problems. I'm also less concerned about the manual copy/paste case - though it seems like if necessary, we should be able to use documentchannel / E10SUtils process selector stuff to ensure the blob URIs load in the same process that created them. In Chrome, none of these interactions seem to work; I'm not suggesting we should leak blob URIs forevermore as soon as they get copied to the clipboard, but it seems fine to at least make their immediate use (while they're still available) work correctly.

Comment 22

[Emilio Cobos Álvarez (:emilio)]

FWIW, it seems to me like we could also not rely on the blob URI at all to implement this stuff, and just create a new blob when requested using drawImage.

Anyhow, just a thought, not sure if it's worth it.

Comment 23

[Anne (:annevk)]

Generally, if a blob URL is not revoked a user should be able to command+click or copy-and-paste it into a fresh tab.

The specific scenarios in this bug pertain UI features. Given that, what Emilio suggests seems appropriate for images drawn from blob URLs. It might even be appropriate to offer that functionality for all drawn images, so the server cannot play tricks on the user (will be rather easy with the Sec-Fetch-Dest header), but I suspect some users do want the actual URL so it might be a hard to design a UI that pleases everyone.

An even more general solution, but not practical due to the current way we handle blob URLs, would be to instruct the URL parser to create a backup blob URL for UI purposes (that cannot be revoked from content) in all places that end up exposing the URL to the user. And revoke those when the UI gets torn down. That way the user can always get to the underlying data.

Hope that helps.

Comment 24

[:Gijs (he/him)]

The issue with the blob URIs we create ourselves is that they leak forever (this is the case today for canvas images where we generate our own blob URIs, and would be expanded if we start using them to "replace" content-originated blobs for <img> tags and similar, too). We'd want to come up with some strategy to decide when to revoke such blob URIs... might be worth a generic follow-up bug?

Comment 25

[Anne (:annevk)]

Okay, so the problem with our <canvas> solution is that the blob URL is effectively never revoked. It does need to be created using the SystemPrincipal as the <canvas> image data might be tainted. Ideally we allow that, but still tie the lifetime of the SystemPrincipal created blob URL to the <canvas>'s or <img>'s node document. Subhamoy, Andrea, thoughts?

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/abbf3817cb8c
Fix HTMLImageElement.currentSrc when we share the underlying blob image data. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/85a3625add23
Make context menu for images properly check for revoked blob URIs. r=Gijs

Comment 27

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/22967 for changes under testing/web-platform/tests

Comment 28

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 29

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/abbf3817cb8c
https://hg.mozilla.org/mozilla-central/rev/85a3625add23

Comment 30

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 31

[Andrea Marchesini [:baku]]

I think we can store the blobURL in the global's store for the canvas. See bug 1630374.

Comment 32

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1625786 - Un-skip some tests now that the underlying issue is fixed.

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0ee4c43ecedf
Un-skip some tests now that the underlying issue is fixed.

Comment 34

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/0ee4c43ecedf

Comment 35

[Clara Guerrero ( Need Info Brindusa Tot Please)]

Hi

I've tested this issue on Windows 10 pro with firefox nightly 78.0a1 (2020-05-08) (64-bit) and firefox beta 77.0b3 (64-bit)and this issue is not reproducible anymore. Based on this I will mark the bug as Verified.

Best,
Clara