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.

We'd need more details to have this bug be actionable. For example, what is actually the suggested bug in background pages?

(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

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.

Verified, and this affects all context menus available on the image that use the url.

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.

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.

(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

So it should be set to the right-clicked DOM node's currentURI, right? Will look at this.

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.

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?

Edgar or Samael may be able to help.

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

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.

(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

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.

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

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

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

Backed out 3 changesets (bug 1406253)for build bustage in dom/base/nsCopySupport.cpp r=backout on a CLOSED TREE https://hg.mozilla.org/integration/autoland/rev/8794becbd0cbab8a7222f34d66b8167a77fa6743 https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=284f3cc2880c32f0646bcfad14f513dc9b8e4a07&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success

Try for windows build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=106ebbb032a3bbffe0d75faefc673f7bc147d97d

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

https://hg.mozilla.org/mozilla-central/rev/76b5c4e2c6a2 https://hg.mozilla.org/mozilla-central/rev/9f13acd8e9e0 https://hg.mozilla.org/mozilla-central/rev/8f9e9de25323