HTMLImageElement currentURI after redirect is pre-redirect uri

RESOLVED FIXED in Firefox 59

Status

()

defect
P2
normal
RESOLVED FIXED
2 years ago
5 months ago

People

(Reporter: bruce.bugz, Assigned: freesamael)

Tracking

(Depends on 1 bug)

58 Branch
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 wontfix, firefox59 fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20170914024831

Steps to reproduce:

Request Control: https://addons.mozilla.org/en-US/firefox/addon/requestcontrol/

I filed this bug on the developer's Github where the dev suggested it might be a "problem with the background API." https://github.com/tumpio/requestcontrol/issues/38

I don't know how to create a simple test case though, so the most minimal STR I can offer are the same as in the issue above. It is still reproducible on the latest Nightly - just install Request Control from the link above and follow the STR on the Github issue.
Component: Untriaged → WebExtensions: General
Product: Firefox → Toolkit
We'd need more details to have this bug be actionable. For example, what is actually the suggested bug in background pages?
Flags: needinfo?(ampersand100000)
(In reply to Andy McKay [:andym] from comment #1)
> We'd need more details to have this bug be actionable. For example, what is
> actually the suggested bug in background pages?

I was only reiterating what the developer of that add-on said[1]. I do not know what, specifically, they had in mind.

FWIW, in an attempt to narrow down the issue, I did the following:

1. Follow the instructions here[2] to create a minimal webextension that redirects requests. I will attach my xpi, but it should basically redirect:
https://mdn.mozillademos.org/files/13537/5-new-request-list-new.png
to
https://38.media.tumblr.com/tumblr_ldbj01lZiP1qe0eclo1_500.gif
2. Go to https://mdn.mozillademos.org/files/13537/5-new-request-list-new.png
3. The address shown in the address bar changes to https://38.media.tumblr.com/tumblr_ldbj01lZiP1qe0eclo1_500.gif and the gif starts playing, as expected.
4. Right click the image > Save Image As > save somewhere.

AR:

A. The suggested file name is `5-new-request-list-new.png.gif` (note how both png and gif are in the file name)
B. The actual file that gets saved is from https://mdn.mozillademos.org/files/13537/5-new-request-list-new.png

ER:

A1. The suggested file name should be `tumblr_ldbj01lZiP1qe0eclo1_500.gif`.
A2. If it should indeed be based on the original URL, then the suggested file name should be `5-new-request-list-new.png` (which is what you get if you didn't have this add-on installed). The suggested extension of `.png.gif` seems to be combining the two extensions.
B. The actual file saved should be from https://38.media.tumblr.com/tumblr_ldbj01lZiP1qe0eclo1_500.gif.

[1]: https://github.com/tumpio/requestcontrol/issues/38#issuecomment-320167681
[2]: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Intercept_HTTP_requests#Redirecting_requests
Flags: needinfo?(ampersand100000)
Posted file Demonstrating webRequests.xpi (obsolete) —
Previous XPI didn't have ID so couldn't be installed outside of downloading first and then using about:debugging.

This one has an ID for easy install.
Attachment #8917198 - Attachment is obsolete: true
Verified, and this affects all context menus available on the image that use the url.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2
Summary: Using the Request Control webextension, trying to save an image from a redirected top-level image page still downloads the non-redirected image → context menus use original url after redirect
Assignee: nobody → mixedpuppy
Ok, the base issue seems to be in HTMLImageElement, more specifically nsIImageLoadingContent.currentURI.  With a clean cache, upon redirection, currentURI is the original pre-redirect url.  If the redirectUrl is a cached image, then currentURI is correctly the redirectUrl.  So, an enhanced STR

- start firefox
- load extension from comment 4.
- open tab and load url https://mdn.mozillademos.org/files/13537/5-new-request-list-new.png
- request is redirected
  - image is the target we expect
  - urlbar has the correct url
* context menus off the image will have the url to 5-new-request-list-new.png
- close tab
- open new tab, load the 5-new-request-list-new.png url again
- request is redirected
* context menus off the image NOW have the correct url (after redirection)

I'm thinking this is a result of bug 1319111.  Maybe Honza will have some insight.
Flags: needinfo?(honzab.moz)
Summary: context menus use original url after redirect → HTMLImageElement currentURI after redirect is pre-redirect uri
Component: WebExtensions: General → Networking
Product: Toolkit → Core
The block of code that I've trace the issue to so far is at

http://searchfox.org/mozilla-central/source/browser/modules/ContextMenu.jsm#808

where mediaURL is set.
Whiteboard: [necko-triaged]
(In reply to Shane Caraveo (:mixedpuppy) from comment #7)
> The block of code that I've trace the issue to so far is at
> 
> http://searchfox.org/mozilla-central/source/browser/modules/ContextMenu.
> jsm#808
> 
> where mediaURL is set.

http://searchfox.org/mozilla-central/rev/ed1d5223adcdc07e9a2589ee20f4e5ee91b4df10/browser/modules/ContextMenu.jsm#808
Flags: needinfo?(honzab.moz)
So it should be set to the right-clicked DOM node's currentURI, right?  Will look at this.
Assignee: mixedpuppy → honzab.moz
So, if the problem is that "Copy Link Location" from the context menu gets https://mdn.mozillademos.org/files/13537/5-new-request-list-new.png instead of https://38.media.tumblr.com/tumblr_ldbj01lZiP1qe0eclo1_500.gif (expected) then that issue precedes bug 1319111.

Not sure what all properties are exposed on img DOM nodes, but I'm sure we also store the original URL (the first one in the redirect chain).  But under "currentURL" I would expect the result URL.

I believe the problem here is in DOM.
Assignee: honzab.moz → mixedpuppy
Component: Networking → DOM
Whiteboard: [necko-triaged]
Andrew, this is an area I'd end up spinning lots of cycles to ramp up, is there someone who might take this on that knows this area well?
Flags: needinfo?(overholt)
Edgar or Samael may be able to help.
Flags: needinfo?(sawang)
Flags: needinfo?(overholt)
Flags: needinfo?(echen)
What I found was that nsImageLoadingContent::GetCurrentURI uses the URI from imgRequest::GetURI [1], while the comment here [2] hinted that URI would be originalURI.

I think nsImageLoadingContent::GetCurrentURI should use imgRequest::GetCurrentURI instead.

[1] http://searchfox.org/mozilla-central/rev/dca019c94bf3a840ed7ff50261483410cfece24f/dom/base/nsImageLoadingContent.cpp#739
[2] http://searchfox.org/mozilla-central/rev/dca019c94bf3a840ed7ff50261483410cfece24f/image/imgLoader.cpp#2565
Flags: needinfo?(sawang)
Flags: needinfo?(echen)
Attachment #8919648 - Flags: review?(bzbarsky)
Hmm.  So nsIImageLoadingContent::GetCurrentURI predates imgIRequest::GetCurrentURI existing by about 10 years.  It's pretty clearly documented as "Gets the URI of the current request", which would correspond to imgIRequest::GetURI.

Note that the notion of "current" in the two cases is different.  For nsIImageLoadingContent it means "current request, not pending request", while for imgIRequest it means "current at this point in time".  The naming collision is deeply unfortunate.
Comment on attachment 8919648 [details]
Bug 1406253 - Part 1: Rename imgIRequest.currentURI to finalURI to prevent confusion.

https://reviewboard.mozilla.org/r/190550/#review195840

::: commit-message-4efa9:5
(Diff revision 1)
> +Bug 1406253 - Use imgRequest::GetCurrentURI instead of GetURI in nsImageLoadingContent::GetCurrentURI, so it wouldn't use originalURI.
> +
> +Using imgRequest::GetURI in nsImageLoadingContent::GetCurrentURI causes that
> +context menu gets originalURI, and saves incorrect file on pressing
> +"Save Image As..." if the image request was redirected to a different URI.

So the key part here is what happens when we have a non-deterministic redirect, right?  That should probably be made clear in the commit message...

::: commit-message-4efa9:7
(Diff revision 1)
> +
> +Using imgRequest::GetURI in nsImageLoadingContent::GetCurrentURI causes that
> +context menu gets originalURI, and saves incorrect file on pressing
> +"Save Image As..." if the image request was redirected to a different URI.
> +
> +As the function name indicated, it should return currentURI instead.

No, the function name indicates that it returns the URI for the current _request_ (as opposed to pending request).  This has nothing to do with the currency of the URI itself.

::: dom/base/nsImageLoadingContent.cpp:739
(Diff revision 1)
>  already_AddRefed<nsIURI>
>  nsImageLoadingContent::GetCurrentURI(ErrorResult& aError)
>  {
>    nsCOMPtr<nsIURI> uri;
>    if (mCurrentRequest) {
> -    mCurrentRequest->GetURI(getter_AddRefs(uri));
> +    mCurrentRequest->GetCurrentURI(getter_AddRefs(uri));

I would like to see an audit of all consumers of this API to ensure that they actually want this change, because this significantly changes the semantics.

No matter what, the documentation needs to be updated.
Attachment #8919648 - Flags: review?(bzbarsky) → review-
Assignee: mixedpuppy → sawang
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #15)
> Note that the notion of "current" in the two cases is different.  For
> nsIImageLoadingContent it means "current request, not pending request",
> while for imgIRequest it means "current at this point in time".  The naming
> collision is deeply unfortunate.

Ah. I see. That sounds I should use request.currentURI.spec in ContextMenu.jsm [1] instead, if request is available.

[1] http://searchfox.org/mozilla-central/rev/d0448c970093f94bd986a21d3a9e8c366b316eb4/browser/modules/ContextMenu.jsm#822
To be clear, I don't have a problem with changing the semantics of currentURI on HTMLImageElement.  I just want us to check that all the consumers want the new semantics.
Just some updates.

I looked at the full list of currentURI keyword on searchfox and found this pattern should cover all js callers (except the 1st one on the list isn't about nsIImageLoadingContent). Most of them look like context menu related - saving/sharing/set as desktop background, etc.

http://searchfox.org/mozilla-central/search?q=(target%7Cnode%7Celem%7Celement).currentURI&case=false&regexp=true&path=

C++ callers. The two under layout/ are mystery to me.

http://searchfox.org/mozilla-central/search?q=symbol:_ZN22nsIImageLoadingContent13GetCurrentURIEPP6nsIURI&redirect=false

I got an assertion on try with the patch applied. Haven't figured out why.

https://treeherder.mozilla.org/logviewer.html#?job_id=137832634&repo=try&lineNumber=36577
> C++ callers. The two under layout/ are mystery to me.

The one in nsDocumentViewer::GetInImage doesn't care what the URI is, just whether it's null.  So it would be OK either way.

SurfaceFromElement likewise just cares about the URI.  Further, it only cares about it in the "no image request" case, when the change you're proposing won't make a difference anyway.

> I got an assertion on try with the patch applied. Haven't figured out why.

Probably because you're changing the behavior of the currentSrc attribute on HTMLImageElement given the test that the assertion is happening in?

That also means that you're changing web-exposed behavior, which is not desirable here: currentSrc definitely needs to be the pre-redirect URI.  See https://html.spec.whatwg.org/multipage/embedded-content.html#dom-img-currentsrc

Note that the specs concept of "current URL" for an image request does NOT match our concept of imgIRequest::GetCurrentURI, by the way.  I really think it might be worth renaming the imgIRequest accessors to reduce the confusion there...
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #20)
> > C++ callers. The two under layout/ are mystery to me.
> 
> The one in nsDocumentViewer::GetInImage doesn't care what the URI is, just
> whether it's null.  So it would be OK either way.
> 
> SurfaceFromElement likewise just cares about the URI.  Further, it only
> cares about it in the "no image request" case, when the change you're
> proposing won't make a difference anyway.
> 
> > I got an assertion on try with the patch applied. Haven't figured out why.
> 
> Probably because you're changing the behavior of the currentSrc attribute on
> HTMLImageElement given the test that the assertion is happening in?
> 

Thanks for the explanation! It would take me serveral days to figure these out without your help.

> That also means that you're changing web-exposed behavior, which is not
> desirable here: currentSrc definitely needs to be the pre-redirect URI.  See
> https://html.spec.whatwg.org/multipage/embedded-content.html#dom-img-
> currentsrc
> 
> Note that the specs concept of "current URL" for an image request does NOT
> match our concept of imgIRequest::GetCurrentURI, by the way.

That's indeed quite confusing. Edgar hinted me that currentSrc can be useful for developers when they're using srcset. That helps me a bit to understand what currentSrc is for. The other interesting thing he found is that unlike currentSrc, the src attribute would be set to the redirected URL with the STR in comment 6, when the ImageDocument is created here:
http://searchfox.org/mozilla-central/rev/1c4da216e00ac95b38a3f236e010b31cdfaae03b/dom/html/ImageDocument.cpp#711

(I'm curious how web developers are able to deal with all these confusing stuff...)

So far it sounds using request.currentURI in context menu is much more preferable to modifying nsIImageLoadingContent::GetCurrentURI.

> I really think it might be worth renaming the imgIRequest accessors to reduce the confusion
> there...

Does resultURI sounds reasonable? I'm not quite good at naming.
(In reply to Samael Wang [:freesamael] from comment #21)
> > I really think it might be worth renaming the imgIRequest accessors to reduce the confusion
> > there...
> 
> Does resultURI sounds reasonable? I'm not quite good at naming.

To be clear, I was thinking to keep imgIRequest.URI, and renme imgIRequest.currentURI to imgIRequest.resultURI.
> the src attribute would be set to the redirected URL with the STR in comment 6

This is very much specific to the ImageDocument case.  Fact of the matter is, if you can observe the src of the image in that document, you can also observe its location.href, which is the same thing.

For images where you can observe the _image_ but may not be same-origin with the url it started loading, observing redirect behavior would be a cross-site information leak, which is why images always return pre-redirect URLs.

> Does resultURI sounds reasonable? 

Or finalURI?
Comment on attachment 8919648 [details]
Bug 1406253 - Part 1: Rename imgIRequest.currentURI to finalURI to prevent confusion.

https://reviewboard.mozilla.org/r/190550/#review205100
Attachment #8919648 - Flags: review?(bzbarsky) → review+
Comment on attachment 8928470 [details]
Bug 1406253 - Part 2: Implement nsIImageLoadingContent.currentRequestFinalURI.

https://reviewboard.mozilla.org/r/199724/#review205102

::: commit-message-1fc8b:3
(Diff revision 1)
> +Bug 1406253 - Part 2: Implement nsIImageLoadingContent.currentRequestFinalURI. r?bz
> +
> +Since nsIImageLoadingContent.currentURI would only return the URI of

How about:

ImageLoadingContent.currentURI returns the "URI" of currentRequest, which is the URI used to start that request.  Some consumers need to know the final URI of that request instead.

::: commit-message-1fc8b:6
(Diff revision 1)
> +Bug 1406253 - Part 2: Implement nsIImageLoadingContent.currentRequestFinalURI. r?bz
> +
> +Since nsIImageLoadingContent.currentURI would only return the URI of
> +currentRequest, add another method for finalURI of currentRequest.
> +
> +If the image request gets non-deterministic redirect on loading (e.g. an add-on

It doesn't matter whether the redirect is deterministic, right?

::: dom/base/nsIImageLoadingContent.idl:165
(Diff revision 1)
>     * null if there haven't been any such attempts.
>     */
>    readonly attribute nsIURI currentURI;
>  
>    /**
> +   * Gets the final URI of the current request, if available.

Is there a reason to add this to the XPCOM interface?  I'd suggest just adding it to the WebIDL interface instead.

::: dom/base/nsImageLoadingContent.cpp:761
(Diff revision 1)
>    *aURI = GetCurrentURI(result).take();
>    return result.StealNSResult();
>  }
>  
> +already_AddRefed<nsIURI>
> +nsImageLoadingContent::GetCurrentRequestFinalURI(ErrorResult& aError)

Why is aError here?  It seems unused...  If we're OK with returning null in the various error conditions (and the IDL suggests we are), then we don't need this at all.

::: dom/webidl/HTMLImageElement.webidl:106
(Diff revision 1)
>    [ChromeOnly,Throws]
>    long getRequestType(imgIRequest aRequest);
>    [ChromeOnly,Throws]
>    readonly attribute URI? currentURI;
>    [ChromeOnly,Throws]
> +  readonly attribute URI? currentRequestFinalURI;

Should be documented here, if we remove it from the .idl file.
Attachment #8928470 - Flags: review?(bzbarsky) → review-
Comment on attachment 8928471 [details]
Bug 1406253 - Part 3: use currentRequstFinalURI in context menu and add a test case.

https://reviewboard.mozilla.org/r/199726/#review205464

::: browser/modules/ContextMenu.jsm:543
(Diff revision 1)
>      // Media related cache info parent needs for saving
>      let contentType = null;
>      let contentDisposition = null;
>      if (aEvent.target.nodeType == Ci.nsIDOMNode.ELEMENT_NODE &&
>          aEvent.target instanceof Ci.nsIImageLoadingContent &&
>          aEvent.target.currentURI) {

Does this need an update?

::: browser/modules/ContextMenu.jsm:549
(Diff revision 1)
>        disableSetDesktopBg = this._disableSetDesktopBackground(aEvent.target);
>  
>        try {
>          let imageCache = Cc["@mozilla.org/image/tools;1"].getService(Ci.imgITools)
>                                                           .getImgCacheForDocument(doc);
>          let props = imageCache.findEntryProperties(aEvent.target.currentURI, doc);

And this?

::: mobile/android/chrome/content/browser.js:854
(Diff revision 1)
>        order: NativeWindow.contextmenus.DEFAULT_HTML5_ORDER - 1, // Show above HTML5 menu items
>        showAsActions: function(aTarget) {
>          let doc = aTarget.ownerDocument;
>          let imageCache = Cc["@mozilla.org/image/tools;1"].getService(Ci.imgITools)
>                                                           .getImgCacheForDocument(doc);
> -        let props = imageCache.findEntryProperties(aTarget.currentURI, doc);
> +        let props = imageCache.findEntryProperties(aTarget.currentRequestFinalURI, doc);

This variable appears to be unused...
Attachment #8928471 - Flags: review?(dao+bmo) → review+
Comment on attachment 8928471 [details]
Bug 1406253 - Part 3: use currentRequstFinalURI in context menu and add a test case.

https://reviewboard.mozilla.org/r/199726/#review205464

> This variable appears to be unused...

Blamed the history and it was originally used for mime type:
https://searchfox.org/mozilla-central/rev/43735732bbe93600b75331229328f0a01364fe24/mobile/android/chrome/content/browser.js#638,642,649

Looks like poeple forgot to remove the line after changed the message. Removed.
Comment on attachment 8928471 [details]
Bug 1406253 - Part 3: use currentRequstFinalURI in context menu and add a test case.

https://reviewboard.mozilla.org/r/199726/#review205944

::: mobile/android/chrome/content/browser.js:853
(Diff revision 2)
>        selector: NativeWindow.contextmenus._disableRestricted("SHARE", NativeWindow.contextmenus.imageShareableContext),
>        order: NativeWindow.contextmenus.DEFAULT_HTML5_ORDER - 1, // Show above HTML5 menu items
>        showAsActions: function(aTarget) {
>          let doc = aTarget.ownerDocument;
>          let imageCache = Cc["@mozilla.org/image/tools;1"].getService(Ci.imgITools)
>                                                           .getImgCacheForDocument(doc);

doc and imageCache are unused too now.
Comment on attachment 8928470 [details]
Bug 1406253 - Part 2: Implement nsIImageLoadingContent.currentRequestFinalURI.

https://reviewboard.mozilla.org/r/199724/#review206098
Attachment #8928470 - Flags: review?(bzbarsky) → review+
Comment on attachment 8928471 [details]
Bug 1406253 - Part 3: use currentRequstFinalURI in context menu and add a test case.

https://reviewboard.mozilla.org/r/199726/#review205944

> doc and imageCache are unused too now.

Oh silly me :/
Pushed by shindli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9ce01198e8a1
Part 1: Rename imgIRequest.currentURI to finalURI to prevent confusion. r=bz
https://hg.mozilla.org/integration/autoland/rev/aecb3d509a39
Part 2: Implement nsIImageLoadingContent.currentRequestFinalURI. r=bz
https://hg.mozilla.org/integration/autoland/rev/284f3cc2880c
Part 3: use currentRequstFinalURI in context menu and add a test case. r=dao
Keywords: checkin-needed
Pushed by rgurzau@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/76b5c4e2c6a2
Part 1: Rename imgIRequest.currentURI to finalURI to prevent confusion. r=bz
https://hg.mozilla.org/integration/autoland/rev/9f13acd8e9e0
Part 2: Implement nsIImageLoadingContent.currentRequestFinalURI. r=bz
https://hg.mozilla.org/integration/autoland/rev/8f9e9de25323
Part 3: use currentRequstFinalURI in context menu and add a test case. r=dao
Keywords: checkin-needed
Depends on: 1452819
Depends on: 1466379
Depends on: 1469996
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.