Displaying images using object URLs is sometimes corrupted

VERIFIED FIXED in Firefox -esr60

Status

()

P2
normal
VERIFIED FIXED
a year ago
9 months ago

People

(Reporter: julienw, Assigned: baku)

Tracking

({regression})

Trunk
mozilla61
regression
Points:
---

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox-esr60 verified, firefox59 wontfix, firefox60+ wontfix, firefox61+ verified)

Details

Attachments

(2 attachments)

(Reporter)

Description

a year ago
Posted file index-simple.html
STR:
1. Load the attachment.
2. Load a lot of images in the same time using either drag and drop or the file input.
=> Sometimes some of the images will display in a corrupted way.

In the testcase we create an object URL to display all images with <img> tags.
 
This was reported to me by a french user (Renaud Chaput, cc-ed). He helped me write a simple testcase, and he bisected himself to bug 1399466, so it should be present since Fx58. He's on MacOSX.

I could reproduce on my side too, on Linux.

Now, the trick is that it seems to be very dependent on the source images, at least for me. For Renaud it seems to happen more easily, about 1% of his tries.
On my own computer I couldn't reproduce with simpler images, like red rectangles of various sizes.

I'd like to set the "tracking" flag because I think this is important for a lot of use cases happening in real life. I'm thinking of photos uploading in print services, or to picasa, for example.
(Reporter)

Updated

a year ago
status-firefox-esr52: --- → unaffected

Comment 1

a year ago
Thanks Julien!

Here is how it looks like for me (MacOS X, Firefox Nightly): https://i.imgur.com/EkN4ahB.jpg (2 corrupted images).
I am able to reproduce it about 50% of the time when loading those files using the file picker: https://www.dropbox.com/sh/gerefcbompm4xk0/AAAj22Wo2EboyIj7wI7VUP5Fa?dl=0 (images from pexels.com, licenced under CC0).
Flags: needinfo?(amarchesini)
(Reporter)

Updated

a year ago
Keywords: regression
status-firefox59: affected → wontfix
tracking-firefox60: ? → +
tracking-firefox61: ? → +

Updated

11 months ago
Assignee: nobody → amarchesini
Priority: -- → P2

Comment 2

11 months ago
Firefox 59.0.2 (64-bit) at windows 10 and I think that is a firefox bug. 
I have tested this issue with two computers and one computer reproduce always good images and the second computer always bad images but the version of Firefox is the same.

Comment 3

11 months ago
Confirmed with Firefox 60.0b4 (64-bit) on Linux. It happens maybe 20% of the times with the test case when I load all images from Renauds Dropbox link: https://i.imgur.com/W1k8P2M.jpg If it happens, it is always more than one image which is corrupt.
(Assignee)

Comment 4

11 months ago
I debugged this issue for a few hours and I have some results and a couple of questions.

First of all, a description of what happens here:
. imgRequest is receiving data in order to display images.
. data is received by a nsInputStreamPump (via a nsIChannel)
. nsInputStreamPump receives data from a pipe

When new data is coming, nsInputStreamPump::OnInputStreamReady is called (because of nsIAsyncInputStream::AsyncWait())
https://searchfox.org/mozilla-central/rev/c0d81882c7941c4ff13a50603e37095cdab0d1ea/netwerk/base/nsInputStreamPump.cpp#392

Here we start a loop:
https://searchfox.org/mozilla-central/rev/c0d81882c7941c4ff13a50603e37095cdab0d1ea/netwerk/base/nsInputStreamPump.cpp#401

OnStateTransfer() is called and imgRequest receives data.

Then, this block should run, in order to break the loop:
https://searchfox.org/mozilla-central/rev/c0d81882c7941c4ff13a50603e37095cdab0d1ea/netwerk/base/nsInputStreamPump.cpp#474-481

But this doesn't happen, because mSuspendCount is not 0.
The loop continues, but the pipe doesn't have anything else to read, ::available() returns 0, and we go in STATE_STOP state.
Result: the image is not shown, or is partially shown.

The question is: why is mSuspendCount not 0? This happens because Suspend() is called, but I don't know enough to know why this happens and how Suspend()/Resume() should interact with nsInputStreamPump.

Following this documentation: https://searchfox.org/mozilla-central/rev/c0d81882c7941c4ff13a50603e37095cdab0d1ea/netwerk/base/nsIRequest.idl#68-82 , nsInputStreamPump should simply 'stop' the delivering of data and without moving to STATE_STOP state. When ::Resume() is called, the delivering should start again.
Flags: needinfo?(amarchesini) → needinfo?(michal.novotny)
(Assignee)

Comment 5

11 months ago
Posted patch suspend.patchSplinter Review
I think this is related to bug 1026951.

1. In this scenario, we are in STATE_TRANSFER.

2. On a background thread we are executing OnStateTransfer(). In the meantime, the pump is suspended.

3. Because of this, we don't call EnsureWaiting(): https://searchfox.org/mozilla-central/rev/c0d81882c7941c4ff13a50603e37095cdab0d1ea/netwerk/base/nsInputStreamPump.cpp#476

4. But there is nothing to read in the inputStream, and the next call to OnStateTransfer() returns STATE_STOP. Follow the selected line: https://searchfox.org/mozilla-central/rev/c0d81882c7941c4ff13a50603e37095cdab0d1ea/netwerk/base/nsInputStreamPump.cpp#552,631,646

5. The next state will be STATE_STOP.

My patch extends what you did in bug 1026951.
Flags: needinfo?(michal.novotny)
Attachment #8973424 - Flags: review?(michal.novotny)
Comment on attachment 8973424 [details] [diff] [review]
suspend.patch

Review of attachment 8973424 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/nsInputStreamPump.cpp
@@ +460,5 @@
>          // Unset mProcessingCallbacks here (while we have lock) so our own call to
>          // EnsureWaiting isn't blocked by it.
>          mProcessingCallbacks = false;
>  
> +        // We must break the loop is suspended during one of the previous

if suspended?
Attachment #8973424 - Flags: review?(michal.novotny) → review+

Comment 7

11 months ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/698ab4eff5bf
nsInputStreamPump should interrupt the reading of the inputStream when suspended, r=michal

Comment 8

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/698ab4eff5bf
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox61: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61

Comment 9

11 months ago
I just tested on my app with the latest Nightly and I am no longer able to reproduce the issue. Thanks!
Could this patch be backported to Firefox 60?
(Reporter)

Comment 10

11 months ago
Hey Renaud,
Firefox 60 will be released in the next few days (I think tomorrow) so we only accept critical patches for this version at the moment.  Moreover this patch is moderatly risky as it touches some base code where even small issues can have a big impact.

The good news is that Firefox 61 is also going to beta soon so the fix for this issue will come soon too!
Agreed that it's too risky for Fx60 at this point. We may want to keep this on the ESR60 radar for consideration after the patch has had some time to bake, however.
status-firefox60: affected → wontfix
status-firefox-esr60: --- → affected
Hello Julien and Renaud,

I have attempted to validate this issue, but I cannot because I can't reproduce it firstly. 
I have tried reproducing it on Windows 10 x64 and Mac OS X 10.12.6 with both the Release version 59.0.3 and 58 (original version) but I was unsuccessful, I cannot validate it's fix while I can't know whether it reproduces on my systems or not. I can also say that I've tried adding more than 200 large photos (jpg and other formats) or a bunch of photos downloaded from pexels.com. The only issue I see is that the images get stretched to fit the designed space.

Can you reproduce it (on any of the versions older than v61) and then validate it's fix in the latest nightly (or any build that contains the fix, newer than v61)?

Thanks!
Flags: needinfo?(renchap)
Flags: needinfo?(felash)

Comment 13

11 months ago
Hi Daniel,

I can confirm I still reproduce the issue with Firefox 60, but it seems fixed in Firefox Nightly, and also in Firefox Beta 61 (tested with Mac OS X 10.13.4).
Flags: needinfo?(renchap)
Thank you Renaud!

Based on the comment above, this issue is now verified.
Status: RESOLVED → VERIFIED
status-firefox61: fixed → verified
Flags: needinfo?(felash)
I can also confirm this fix on Beta 61.0b4, under Win 10 x64 and Ubuntu 16.04 x64.
Flags: qe-verify+
(Assignee)

Updated

10 months ago
Duplicate of this bug: 1434496
Mind requesting uplift if you think we should fix this in 60.1esr?
Flags: needinfo?(amarchesini)
(Assignee)

Comment 18

10 months ago
Comment on attachment 8973424 [details] [diff] [review]
suspend.patch

[Approval Request Comment]
User impact if declined: some images could be truncated when loaded from blobURLs (and in other contexts)
Fix Landed on Version: 61
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch:  none
Flags: needinfo?(amarchesini)
Attachment #8973424 - Flags: approval-mozilla-esr60?
Comment on attachment 8973424 [details] [diff] [review]
suspend.patch

fix a regression with blob urls, verified in 61, approved for 60.1esr
Attachment #8973424 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+

Comment 20

10 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-esr60/rev/1816f74764fe
status-firefox-esr60: affected → fixed
This is also verified fixed on 60.1.0esr (20180619173714) on the following OSes: Win 7 x64, macOS 10.13 and Ubuntu 16.04 x64.
status-firefox-esr60: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.