Closed Bug 1457532 Opened 6 years ago Closed 6 years ago

ImageLoader prematurely unblocks Document onload in response to a LOAD_COMPLETE event

Categories

(Core :: Graphics: ImageLib, defect, P3)

Product:
Core
Component:
Graphics: ImageLib
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: bradwerth, Assigned: bradwerth)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(2 files)

Bug 1457532 Part 1: Narrow ImageLoader::OnLoadComplete to only unblock onload in response to an error status on the image request.
59 bytes, text/x-review-board-request
tnikkel
: review+
Details
Bug 1457532 Part 2: Change ImageLoader::AssociateRequestToFrame to kickoff decode for images that block onload.
59 bytes, text/x-review-board-request
tnikkel
: review+
Details 
The code in ImageLoader::onLoadComplete assumes that LOAD_COMPLETE is the last event fired by an ImageRequest. This is not the case. Some image loads send FRAME_COMPLETE events after a LOAD_COMPLETE event. By acting on this incorrect assumption, Document onload can be unblocked prematurely, leading to intermittent test failures, such as:

From Bug 1454835: https://treeherder.mozilla.org/logviewer.html#?job_id=174183338&repo=autoland&lineNumber=10237
From Bug 1457132: https://treeherder.mozilla.org/logviewer.html#?job_id=175666664&repo=mozilla-central&lineNumber=10735

And is also the reason for the intermittent failures in Bug 1454694.

The correct behavior is for Document onload to be unblocked in all these cases:
1) When the event sequence is FRAME_COMPLETE -> LOAD_COMPLETE, after reflow is complete (this is working correctly).
2) When the event sequence is LOAD_COMPLETE -> FRAME_COMPLETE, after reflow is complete (this is not working).
3) When the event sequence is LOAD_COMPLETE with an error status code (this is working only because of invalid assumptions in case 2).
Blocks: 1454835
Attachment #8971670 - Flags: review?(tnikkel)
Comment on attachment 8971670 [details]
Bug 1457532 Part 1: Narrow ImageLoader::OnLoadComplete to only unblock onload in response to an error status on the image request.

https://reviewboard.mozilla.org/r/240438/#review246660

Don't we also need to check in AssociateRequestToFrame if the imgrequest has error status in which case we do not want to block load?

Also, do you have something that requests a decode of these images? If nothing requests a decode you can not expect a frame complete notification.
Attachment #8971670 - Flags: review?(tnikkel)
Comment on attachment 8971670 [details]
Bug 1457532 Part 1: Narrow ImageLoader::OnLoadComplete to only unblock onload in response to an error status on the image request.

https://reviewboard.mozilla.org/r/240438/#review246660

The patch now checks request error status in ImageLoader::AssociateRequestToFrame, and will not block onload if the request has an error.

Decode for these images is triggered during reflow in nsFloatManager::ShapeInfo::CreateImageShape.
Comment on attachment 8971670 [details]
Bug 1457532 Part 1: Narrow ImageLoader::OnLoadComplete to only unblock onload in response to an error status on the image request.

https://reviewboard.mozilla.org/r/240438/#review248112
Attachment #8971670 - Flags: review?(tnikkel) → review+
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/754824066ed0
Narrow ImageLoader::OnLoadComplete to only unblock onload in response to an error status on the image request. r=tnikkel
Priority: -- → P3
Whiteboard: [gfx-noted]
Attachment #8974165 - Flags: review?(tnikkel)
(In reply to Timothy Nikkel (:tnikkel) from comment #4)
> Also, do you have something that requests a decode of these images? If
> nothing requests a decode you can not expect a frame complete notification.

You're right. The code was vulnerable to decode never being triggered. This is what caused my landing attempt to fail. I've added a Part 2 to the patch that triggers decode when onload is blocked, if the frame data is not already available.
Flags: needinfo?(bwerth)
Comment on attachment 8974165 [details]
Bug 1457532 Part 2: Change ImageLoader::AssociateRequestToFrame to kickoff decode for images that block onload.

https://reviewboard.mozilla.org/r/242444/#review249748

::: layout/style/ImageLoader.cpp:122
(Diff revision 1)
>            RequestReflowOnFrame(fwfToModify, aRequest);
> +        } else {
> +          // If we don't already have a complete frame, kickoff decode. This
> +          // will ensure that either onFrameComplete or onLoadComplete will
> +          // unblock document onload.
> +          aRequest->StartDecoding(imgIContainer::FLAG_ASYNC_NOTIFY);

StartDecoding will do some decoding synchronously on the main thread. This is a waste of time since we won't get the frame complete notification until we go back to the event loop anyways. StartDecoding is mainly for during painting when we can try to draw the image without getting notifications.

Instead get an imgIContainer from the imgRequestProxy and call RequestDecodeForSize on it. You can pass any size for the size because it will be ignored as long as you don't pass the high quality scaling flag.
Attachment #8974165 - Flags: review?(tnikkel)
(In reply to Timothy Nikkel (:tnikkel) from comment #15)
> Comment on attachment 8974165 [details]
> Bug 1457532 Part 2: Change ImageLoader::AssociateRequestToFrame to kickoff
> decode for images that block onload.
> 
> https://reviewboard.mozilla.org/r/242444/#review249748
> 
> ::: layout/style/ImageLoader.cpp:122
> (Diff revision 1)
> >            RequestReflowOnFrame(fwfToModify, aRequest);
> > +        } else {
> > +          // If we don't already have a complete frame, kickoff decode. This
> > +          // will ensure that either onFrameComplete or onLoadComplete will
> > +          // unblock document onload.
> > +          aRequest->StartDecoding(imgIContainer::FLAG_ASYNC_NOTIFY);
> 
> StartDecoding will do some decoding synchronously on the main thread. This
> is a waste of time since we won't get the frame complete notification until
> we go back to the event loop anyways. StartDecoding is mainly for during
> painting when we can try to draw the image without getting notifications.
> 
> Instead get an imgIContainer from the imgRequestProxy and call
> RequestDecodeForSize on it. You can pass any size for the size because it
> will be ignored as long as you don't pass the high quality scaling flag.

I'm not able to get this approach to work correctly. The imgRequestProxy objects might not have images yet, and so converting them into imgIContainers is just getting NULL pointers. In such a case -- which is the case encountered in the failing mochitest layout/style/test/test_value_cloning.html -- calling aRequest->StartDecoding() is just calling imgRequest::StartDecoding(), which doesn't appear to trigger synchronous decode.

For this reason, I'll do a hybrid approach that attempts the conversion to imgIContainer, and if that fails, falls back to StartDecoding since the image doesn't exist and the imgRequest::StartDecoding() call appears harmless.
Comment on attachment 8974165 [details]
Bug 1457532 Part 2: Change ImageLoader::AssociateRequestToFrame to kickoff decode for images that block onload.

https://reviewboard.mozilla.org/r/242444/#review250124

Thanks!

Sorry I should have explained that the img container can be null.
Attachment #8974165 - Flags: review?(tnikkel) → review+
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/24f66f4c48bd
Part 1: Narrow ImageLoader::OnLoadComplete to only unblock onload in response to an error status on the image request. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/e5b9e14935ab
Part 2: Change ImageLoader::AssociateRequestToFrame to kickoff decode for images that block onload. r=tnikkel
https://hg.mozilla.org/mozilla-central/rev/24f66f4c48bd
https://hg.mozilla.org/mozilla-central/rev/e5b9e14935ab
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: