Closed Bug 1161599 Opened 5 years ago Closed 4 years ago

"Reload Image" sometimes missing from contextual menu when image fails to load

Categories

(Firefox :: Menus, defect)

All
Linux
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 51
Tracking Status
e10s - ---
firefox51 --- verified

People

(Reporter: pwd.mozilla, Unassigned)

References

()

Details

(Keywords: regression, Whiteboard: [gfx-noted])

Attachments

(1 file)

The ability to reload an image without reloading the whole page is missing from the contextual menu with e10s enabled.
Paul, can you reproduce this with a fresh profile (or with all your addons disabled)?
Flags: needinfo?(pwd.mozilla)
(In reply to [:tracy] Tracy Walker - QA Mentor from comment #1)
> Paul, can you reproduce this with a fresh profile (or with all your addons
> disabled)?

With all add-ons disabled I can reproduce it. I haven't tried with a fresh profile.
Flags: needinfo?(pwd.mozilla)
Hey Ian - feel like taking a crack at this one?
Flags: needinfo?(moz-ian)
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #3)
> Hey Ian - feel like taking a crack at this one?
I can take a look.

Paul could you show me an example of where you are seeing this behaviour please?  The testing page I used for bug 1141160 is still causing "Reload Image" to be shown in e10s.
http://kwan.perix.co.uk/mozilla/cpow/cpowReloadImageTest.html
Flags: needinfo?(moz-ian) → needinfo?(pwd.mozilla)
It works on the page you linked to, but Tracy and my avatar's on this page aren't displaying the contextual menu item. Could it be to do with hyperlinked images? Which would fit with where I'm usually seeing images on my travels.
Flags: needinfo?(pwd.mozilla)
(In reply to Paul [pwd] from comment #5)
> It works on the page you linked to, but Tracy and my avatar's on this page
> aren't displaying the contextual menu item. Could it be to do with
> hyperlinked images? Which would fit with where I'm usually seeing images on
> my travels.

You mean you are seeing "Reload Image" on them in non-e10s?  I don't believe that should be happening, since the item shouldn't be there for images that have loaded correctly.  I'm certainly not seeing it on those avatars in release or in non-e10s nightly.
Flags: needinfo?(pwd.mozilla)
Sorry, Tracy's avatar hasn't appeared here on bugzilla since filing this bug and then as Sod's law would have it, after my last response, both his and mine came back up again. Do you have a test case where the image is wrapped in a hyperlink tag, as I've been looking for another test case and images seem to be loading everywhere perfectly right now.
Flags: needinfo?(pwd.mozilla)
I've updated the linked test to include one wrapped in a link, and I'm still seeing the menuitem.  It might have been dependent on exactly why the images weren't loading for you, which'll be harder to debug.
Flags: needinfo?(pwd.mozilla)
The new test case worked also, luckily I thought I'd give it a try tracking down one which showed the issue and got lucky. The video is too large to attach here, but here's the dropbox. Excuse the fact it was done from my phone: https://www.dropbox.com/s/fchq9hn16j8wiqp/20150508_104031.mp4?dl=0
Flags: needinfo?(pwd.mozilla)
And you definitely did see the menu item in non-e10s?

Do you know why the image didn't load the first time?  This isn't really possible to debug without recreating the network conditions that cause stuff not to load for you.  If I make the domain the image is on resolve to ::1 using the hosts file to make the image not load, I still don't get the "Reload Image" option in release.
Flags: needinfo?(pwd.mozilla)
(In reply to Ian Moody [:Kwan] from comment #10)
> And you definitely did see the menu item in non-e10s?
100%

> Do you know why the image didn't load the first time?  This isn't really
> possible to debug without recreating the network conditions that cause stuff
> not to load for you.  If I make the domain the image is on resolve to ::1
> using the hosts file to make the image not load, I still don't get the
> "Reload Image" option in release.

I've been checking this out over the past couple of days. It seems that the image just doesn't get requested properly. For example I'll open a page and I'll see the broken image thing pretty much straight away. I'll also see the same issue with tabs that are being restored where the image doesn't load.
Flags: needinfo?(pwd.mozilla)
(In reply to Paul [pwd] from comment #11)
> (In reply to Ian Moody [:Kwan] from comment #10)
> > And you definitely did see the menu item in non-e10s?
> 100%
> 
> > Do you know why the image didn't load the first time?  This isn't really
> > possible to debug without recreating the network conditions that cause stuff
> > not to load for you.  If I make the domain the image is on resolve to ::1
> > using the hosts file to make the image not load, I still don't get the
> > "Reload Image" option in release.
> 
> I've been checking this out over the past couple of days. It seems that the
> image just doesn't get requested properly. For example I'll open a page and
> I'll see the broken image thing pretty much straight away. I'll also see the
> same issue with tabs that are being restored where the image doesn't load.

Hmm.  This is probably getting a bit beyond me, but I have one last possibly-helpful debugging step I can suggest.
The next time this happens, open the Browser Toolbox https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox , use the inspector in there to inspect the offending image, and then in the console of the toolbox run the code:
let request = $0.getRequest(Components.interfaces.nsIImageLoadingContent); request.imageStatus

do that for both e10s and non-e10s and post the values back here (they should be simple integers)
The Browser Toolbox Inspector wouldn't let me inspect page elements and the normal Inspector would allow it, but when I attempted to run the command in the console it would return "TypeError: $0.getRequest is not a function".
Could this be linked to the way that e10s interacts with the cache?
(In reply to Paul [pwd] from comment #13)
> The Browser Toolbox Inspector wouldn't let me inspect page elements and the
> normal Inspector would allow it, but when I attempted to run the command in
> the console it would return "TypeError: $0.getRequest is not a function".
Ah, right, of course, the Browser Toolbox can't reach content during e10s.  I forgot.  And the Browser Content Toolbox doesn't have an Inspector...

hmm, slightly more convoluted way, but seems to work, next time the problem happens:
- Open the Browser Content Toolbox (BCT)
- Go to the Debugger tab in the BCT
- find the file content.js (in chrome://browser section)
- set a breakpoint at the line "  let defaultPrevented = event.defaultPrevented;" (right after "let handleContentContextMenu = function (event) {"), should be around line 62 of the file.
- right-click on the offending image in the browser, and the Debugger in the BCT should get auto-focused
- switch to the console tab in BCT, run this code:
let request = event.target.getRequest(Components.interfaces.nsIImageLoadingContent); request.imageStatus
Assignee: nobody → moz-ian
Assignee: moz-ian → felipc
So I managed to build a testcase that works reliably to reproduce this. See URL. While the images are loading the context menu appear, but they don't show up after they are in a broken state.

With the testcase I verified that this is a not an e10s issue, as it happens both in e10s and non-e10s windows.

I then bisected using mozregression and found the following range:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2114ef80f6ae&tochange=b62ccf3228ba

The most suspect bug in that range is bug 1084136, which reworked the image load status code.
Assignee: felipc → nobody
Blocks: 1084136
Status: UNCONFIRMED → NEW
Component: General → ImageLib
Ever confirmed: true
Product: Firefox → Core
Summary: [e10s] "Reload Image" missing from contextual menu → "Reload Image" sometimes missing from contextual menu when image fails to load
(In reply to :Felipe Gomes (needinfo me!) from comment #16)
> 
> The most suspect bug in that range is bug 1084136, which reworked the image
> load status code.

:seth, can you take a look? You worked bug 1084136. Thanks.
Flags: needinfo?(seth)
Whiteboard: [gfx-noted]
:tn can you comment to the bug?
Flags: needinfo?(tnikkel)
My use case is fairly simple and more common. I press Escape key (to cancel loading of ads & heavy page itself) when page is still loading, once I see see that image is available (even if not loaded).

BTW, I can reproduce the bug on this page:   data:text/html,<img src="http://asdf.asdf" alt="zxcv">
The "Reload Image" menu item checks if imgIRequest::STATUS_LOAD_COMPLETE is set.[1][2]
After bug 1084136, imgIRequest::STATUS_LOAD_COMPLETE will be set even if the image has an error. I think this change is intentional because the aim of bug 1084136 is making the image state monotonic. So the frontend side should adapt the check.

[1] https://dxr.mozilla.org/mozilla-central/rev/1a5b53a831e5a6c20de1b081c774feb3ff76756c/browser/base/content/nsContextMenu.js#698-699
[2] https://dxr.mozilla.org/mozilla-central/rev/1a5b53a831e5a6c20de1b081c774feb3ff76756c/browser/base/content/nsContextMenu.js#278
Component: ImageLib → Menus
Flags: needinfo?(tnikkel)
Flags: needinfo?(seth.bugzilla)
Product: Core → Firefox
Seth, please teach me if my guess is wrong.
Flags: needinfo?(seth.bugzilla)
Keywords: regression
Comment on attachment 8785780 [details]
Bug 1161599 - Show "Reload Image" if the image has an error.

https://reviewboard.mozilla.org/r/74852/#review73570

Looks good to me.
Attachment #8785780 - Flags: review?(florian) → review+
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/6233844593ae
Show "Reload Image" if the image has an error. r=florian
https://hg.mozilla.org/mozilla-central/rev/6233844593ae
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Flags: qe-verify+
Flags: needinfo?(seth.bugzilla)
Reproduced the initial issue using old Nightly from 2016-08-30. Verified that Reload images appears in Context menu every time an image fails to load using Firefox 51 beta 10 across platforms (Windows 10 64bit, Mac OS X 10.12.1 and Ubuntu 16.04 64bit).
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.