Daniel Holbert [:dholbert] Reporter
	Description • 10 years ago

I'm noticing this warning being particularly spammy during reftest runs of my "object-fit" reftests (yet-to-land, as part of bug 624647):
{
WARNING: Recursively notifying in RasterImage::FinishedSomeDecoding!: file $SRC/image/src/RasterImage.cpp, line 3059
}

Looking at a recent debug reftest log[1], it looks like it's pretty spammy with existing tests, too. (1478 copies of the warning)

[1] https://tbpl.mozilla.org/php/getParsedLog.php?id=51804996&tree=Mozilla-Inbound

	Daniel Holbert [:dholbert] Reporter
	Comment 1 • 10 years ago

Seth, is this something that's useful to warn about, or can we just drop the warning?

Based on the comment alongside it, it sounds like we expect to handle this scenario correctly, which suggests to me that we should drop the warning. But maybe this code-path is indicative of something going wrong [in which case maybe it's useful after all]?

MXR reference:
http://mxr.mozilla.org/mozilla-central/source/image/src/RasterImage.cpp?rev=a13927f78353&mark=3059-3059#3054

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 2 • 10 years ago

One option is to start using gfxDebug(), though it may need a few patches from bug 1074952 (part 6), which will reduce the chance of seeing this when you're not looking for it...

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 3 • 10 years ago

Maybe we should use gfxDebug() for this, indeed.

The code that prints this warning *is* warning about a potential bug. I hope to remove that code in bug 910441 if it turns out to be relatively straightforward to do so. Previous to that bug, it wasn't.

The warning actually warns about a superset of the truly problematic cases. What we really care about is code that fails if we don't deliver recursive notifications *synchronously*. This just should not ever be required - it's clearly wrong. It shouldn't matter whether the imgINotificationObserver notifications are delivered synchronously or asynchronously, and any code that relies on that is relying on an implementation detail that imagelib shouldn't be forced to maintain.

It may be that bug 910441 will eliminate the need for this warning, as I mentioned above, so I don't think we should take any action right now. Let's make this bug depend on bug 910441 and see whether we can successfully remove the recursive notification code. If not, then we can look at making the warning quieter until we can fix the bad observers.

I think what we should do is make this bug depend on bug

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 4 • 10 years ago

(Whoops, ignore that last line.)

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 5 • 10 years ago

So there's recently been a lot of interest in making data URI and blob URI Image objects drawable into a canvas synchronously, even before their DOM-level load event has fired. I suspect that if we want to support that, we're going to have to make sure that we can always deliver all our notifications synchronously, so it's probably not feasible to remove this code.

This patch just removes the warning.

	Timothy Nikkel (:tnikkel)
	Comment 6 • 10 years ago

Comment on attachment 8522627 [details] [diff] [review]
Remove RasterImage::FinishedSomeDecoding recursive notification warning

Can still be handy sometimes when one is debugging issues around notifications, but definitely spammy.

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 7 • 10 years ago

Once bug 1074952 lands, I may file a follow up bug and put this back in as a gfxDebug() that'd be run time switchable in the debug builds...

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 8 • 10 years ago

Thanks for the review, Timothy!

(In reply to Milan Sreckovic [:milan] from comment #7)
> Once bug 1074952 lands, I may file a follow up bug and put this back in as a
> gfxDebug() that'd be run time switchable in the debug builds...

Yeah, that'd be great; having it switchable would tip the balance in favor of including it.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 9 • 10 years ago

Pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/582d9b5011f9

	Carsten Book [:Tomcat]
	Comment 10 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/582d9b5011f9