1063973 - Load event fires too early for SVG DataURL with DataURL Image

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Hans Schmucker]

Attachment: about:support

This issue first appears in tinderbox build 1409452634. It is not present in build 1409416875. It is hard to reproduce across different machines, since it seems to be very timing dependent.

Basically, under some circumstances, the load event on an HTMLImageElement may fire before the SVG document that it's supposed to show has decoded all images. This is significant to me, because it blocks me from reliably applying SVG filters to content that will be rendered back to a Canvas 2D Context.

At the linked URL is a the simplest example that would still exhibit the issue. It's supposed to move one image around while you move the mouse around, then show a copy below with a 200ms delay. It may or may not work for you. Even editing the image seems to make the issue go away for me.

Here are the steps needed to produce the issue:
*Draw an image onto a Canvas
*Draw that Canvas to a second Canvas (with transforms)
*Call toDataURL on that canvas
*Wrap it in a SVG Image within a SVG document (as text)
*Creat an IMG element
*Attach a load handler which would draw the IMG to a third canvas
*Set the SVG document as SRC of the IMG element

Expected result:
*It should reliably render a copy of the image to the third canvas

Actual result:
*Sometimes the image comes out blank.

This might be related to bug #1008942 ... at least that's the only bug linked to landings in that timeframe that seem even vaguely related (changes to http://dxr.mozilla.org/mozilla-central/source/content/base/src/nsImageLoadingContent.cpp ), but since that bug is locked I have no way of knowing.

The issue is really very, very hard to reproduce as it seems very, very timing dependent. Removing the filter tag (which is not applied to anything and empty) from the SVG source fixes the issue. Changing about 3% of the image to different colors fixes the issue. Changing the zoom used while drawing to the second canvas fixes the issue. Changing the delays fixes the issue. Changing the size of the canvas by more than about 20% does. Pretty much any change does.

Firefox was installed in a fresh directory each time with a fresh and different profile. All plugins are disabled, as is any antivirus software. about:support info is attached.

Comment 1

[Hans Schmucker]

Setting image.mem.decodeondraw AND image.mem.discardable to false also seems to fix the issue.

Comment 2

[Robert Longson [:longsonr]]

Could you attach a testcase please rather than just describing the steps.

Comment 3

[Hans Schmucker]

It's linked at the given URL. Just changed it so that it runs and checks automatically.

Comment 4

[Hans Schmucker]

Attachment: Just an image used for testing.

Comment 5

[Timothy Nikkel (:tnikkel)]

(In reply to Hans Schmucker from comment #0)
> This issue first appears in tinderbox build 1409452634. It is not present in
> build 1409416875.

That implies these changesets
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=82e1c0a8c589&tochange=7ace84a1570b

Comment 6

[Hans Schmucker]

Attachment: delayedSVGTest.html

Testcase. Better use the one at the URL, since that's easier to update, but here's a current snapshot.

Comment 7

[Loic]

Yes, probably due to:
Timothy Nikkel — Bug 1008942. When a network request for an image finishes during paint suppression and the image doesn't have a frame don't start a decode. r=mats Either the image won't get a frame, or it will get a frame very soon and that frame will check its own visibility and kick off a decode if needed.

Comment 8

[Timothy Nikkel (:tnikkel)]

As far as I can tell from the spec (http://www.whatwg.org/specs/web-apps/current-work/multipage/embedded-content.html#the-img-element) the load event only implies that we have received all the data for the image. It doesn't say whether it has to be decoded or not. Now canvas.drawImage should be synchronously decoding the images it needs, and as far as I can tell based on code inspection the sync decode flag should be getting passed through. Maybe we are hitting the canvas image cache or the imagelib surface cache and one of them isn't getting invalidated when it should?

Hans, to try to make this easier to reproduce you could try doing drawImage on an svg image that contains raster images that are never drawn, and there are no references to either image when the document loads. So say add the image dynamically after load with display: none; and then drawImage.

Comment 9

[Hans Schmucker]

Attachment: Simplified testcase, ~50% accuracy

Added a much simplified testcase... it's not quite as reliable and since the images are not generated dynamically, you'll have to close Firefox and re-start it with a new profile to see the issue after each run (no matter whether it was successful or not... force-reloading and any number of cache settings don't seem to help), but it is a lot simpler.

Comment 10

[Timothy Nikkel (:tnikkel)]

Hans, could you try these two builds?

http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-win32/1409424316/
http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-win32/1409421612/

That will confirm the regression window. Thanks.

Comment 11

[Hans Schmucker]

Just tried it... I can confirm that 1409421612 works (does not produce any errors), while 1409424316 is broken.

Comment 12

[Timothy Nikkel (:tnikkel)]

So what's happening here is that a decoding thread is in the Decoder::Write call in RasterImage::WriteToDecoder, so mInDecoder is true, and the main thread is in RasterImage::Draw (with the sync decode flag), so we hit the early exit if checking for sync decode and mInDecoder. This is not what the mInDecoder flag is trying to prevent. I think it was created when image decoding was main thread only, so I think it's purpose is to prevent decoding from notifying which causes a Draw when this does more decoding.

Comment 13

[Timothy Nikkel (:tnikkel)]

Bug 1089046 removes mInDecoder and so should fix this bug.

Comment 14

[Timothy Nikkel (:tnikkel)]

Bug 1089046 landed, so this should be fixed now on nightly (version 36).

Comment 15

[Timothy Nikkel (:tnikkel)]

I just tested nightly and indeed the testcase is now fixed for me. Unfortunately the fix won't make it's way to release until version 36 though unless bug 1089046 gets uplifted, which I don't think there are any plans for at this point.