Closed Bug 727700 Opened 8 years ago Closed 8 years ago

Suppress image invalidations once the decoder hits a data error

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED WONTFIX

People

(Reporter: rclick, Assigned: rclick)

References

Details

Attachments

(1 file)

We don't flush image invalidations from RasterImage if there's a data error (see Bug 726004). This check should happen in the decoder superclass to ensure that we don't flush from Decoder::PostFrameStop() either.

This could possibly fix the random orange in Bug 721917.
Assignee: nobody → rclickenbrock
Status: NEW → ASSIGNED
Attached patch patchSplinter Review
This should do it.

I haven't tested this at all yet as I'm waiting for my build to finish. Building takes forever on my box, so I probably won't come back to this until tomorrow.
Robert, do you have level-one commit access?  I can help you get that so you can push to try.

http://www.mozilla.org/hacking/commit-access-policy/
Robert, what's the status with getting L1 access, etc?  I'd love to get this fix in before we branch.

Let me know if and how I can help.
Sorry for the delay here. My L1 access is bug 728406. That's waiting for server-ops to do their part.

I've been looking at the BMP decoder and I'm starting to suspect that invalidation isn't the problem. Specifically, the FlushInvalidations() call only does something if the decoder first calls PostInvalidation() to post a region to invalidate. As I'm reading it, the BMP decoder should be bailing with a data error before that happens...

I'll investigate further with a debugger and see if I can figure out what's going on.
Summary: Decoder::FlushInvalidations() should return early when there's a data error → Suppress image invalidations once the decoder hits a data error
I can reproduce the test failures on my WinXP box (albeit not reliably).I've confirmed that we're not actually invalidating anything so the patch here won't make any difference. WONTFIX'ing based on that.

I have noticed that on the failing runs we're firing onload instead of onerror. I filed Bug 734681 to fix that.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.