Closed Bug 1565542 Opened 3 months ago Closed 3 months ago

img.decode() never resolved/rejected if src already as html-image and in cache

Categories

(Core :: ImageLib, defect, P2)

68 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla70
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 68+ verified
firefox68 + verified
firefox69 + verified
firefox70 + verified

People

(Reporter: tobias.buschor, Assigned: tnikkel)

References

Details

(Keywords: regression)

Attachments

(1 file)

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

Steps to reproduce:

html:
<img src="https://imagizer.imageshack.com/v2/285x280q90/c/661/ggZf2A.jpg">
javaascript:
img1.src = 'https://imagizer.imageshack.com/v2/285x280q90/c/661/ggZf2A.jpg';
img1.decode().then(function(){
log('loaded 1')
});
See here:
https://jsbin.com/fudowariki/1/edit?html,js,output

Actual results:

The decode promise will not resolve/reject if a html-image-element with the same src already exists and the image is in the cache.

As this future is already shipped, it can break websites using this api.
Code like this will fail:

if (!img.decode) img.onload = function(){...}
img.src = 'http....';
if (img.decode) img.decode().then(...)

Blocks: 1501794
Summary: img.decode() never resolved/rejected if src already in html and in cache → img.decode() never resolved/rejected if src already as html-image and in cache

Can't always be reproduced.
It occurs when DevTools are open and under network the cache is deactivated and reactivated.

I was able to reproduce just by refreshing a few times, no need to fiddle with dev tools or the network cache. This is concerning.

Status: UNCONFIRMED → NEW
Component: Untriaged → ImageLib
Ever confirmed: true
Flags: needinfo?(aosmond)
Priority: -- → P2
Product: Firefox → Core

When we reload the document the destruction of the old document triggers a discard request for the image. If timing is right we haven't locked the image in the new document yet so it discards.

We call LoadImage on the image, it returns the existing entry from the image cache, but it needs to validate. When it validates we send out all the progress in the progress tracker already. This includes frame complete and decode complete even though we have no decoded surfaces for this image right now.

The RequestDecodeForSize call in nsImageLoadingContent::MaybeResolveDecodePromises triggers a decode. When the decode finishes we send a frame update notification but we never send frame complete or decode complete because those are permanent once they happen.

Assignee: nobody → tnikkel
Duplicate of this bug: 1567535
Flags: needinfo?(aosmond)

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Keywords: regression
Pushed by tnikkel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8b0b216b8675
We need to check to resolve image decode promises when we get the frame update notification too. r=aosmond
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70

Here is another proof of this bug: https://codepen.io/anon/pen/eqvbQy . Simply reload the pen. Ideally you should always see the image and a "Decoding done" but more often then not, the image is not seen, because "decode" never finishes.

Tested with User Agent "Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:68.0) Gecko/20100101 Firefox/68.0" - latest public Firefox "68.0.1 (64-Bit)" version on mac os.

Please nominate this for Beta/Release/ESR68 approval when you get a chance.

Flags: needinfo?(tnikkel)

Comment on attachment 9081071 [details]
Bug 1565542. We need to check to resolve image decode promises when we get the frame update notification too. r?aosmond

Beta/Release Uplift Approval Request

  • User impact if declined: img.decode promises sometimes don't get resolved. affects google maps loaded in iframes
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): We just add another trigger to check if we meet the conditions to resolve the promise.
  • String changes made/needed: none

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: causes google maps in an iframe to not work sometimes
  • User impact if declined: google maps in an iframe does not work sometimes
  • Fix Landed on Version: 70
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): We just add another trigger to check if we meet the conditions to resolve the promise.
  • String or UUID changes made by this patch: none
Flags: needinfo?(tnikkel)
Attachment #9081071 - Flags: approval-mozilla-release?
Attachment #9081071 - Flags: approval-mozilla-esr68?
Attachment #9081071 - Flags: approval-mozilla-beta?

Comment on attachment 9081071 [details]
Bug 1565542. We need to check to resolve image decode promises when we get the frame update notification too. r?aosmond

Fixes a known regression in Fx68 causing problems with Google Maps in some cases. Let's get this into tomorrow's 69.0b10 build so it can get some wider testing before next week's dot releases.

Attachment #9081071 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
QA Whiteboard: [qa-triaged]

Confirmed the loading/decode issue with 68.0.1 /69.0b9 after several page reloads.
Fix verified with 70.0a1 (2019-07-31), 69.0b10(taskCluster build) on Windows 10, macOS 10.14, Ubuntu 16.04LTS.

Status: RESOLVED → VERIFIED

Comment on attachment 9081071 [details]
Bug 1565542. We need to check to resolve image decode promises when we get the frame update notification too. r?aosmond

approved for 68.0.2

Attachment #9081071 - Flags: approval-mozilla-release?
Attachment #9081071 - Flags: approval-mozilla-release+
Attachment #9081071 - Flags: approval-mozilla-esr68?
Attachment #9081071 - Flags: approval-mozilla-esr68+

Verified with the task_cluster builds 68.0.2 and the 68.0esr on the avialable OSs.

You need to log in before you can comment on or make changes to this bug.