Closed Bug 1454694 Opened 7 years ago Closed 6 years ago

Re-enable layout/style/test/test_shape_outside_CORS.html

Categories

(Core :: Layout: Floats, enhancement, P3)

Product:
Core
Component:
Layout: Floats
Type:
enhancement

Tracking

()

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

People

(Reporter: bradwerth, Assigned: bradwerth)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

Bug 1454694: Re-enable the timing-sensitive part of the CORS mode test.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details 
Bug 1451196 disabled this test due to intermittent behavior.
Blocks: 1454835
Attachment #8970653 - Flags: review?(bzbarsky)
Attachment #8970749 - Flags: review?(bzbarsky)
Attachment #8970654 - Flags: review?(bzbarsky)
Comment on attachment 8970653 [details]
Bug 1454694 Part 0: Add an imgIRequest status flag indicating that the request is done sending events to observers.

https://reviewboard.mozilla.org/r/239396/#review245504
Attachment #8970653 - Flags: review?(bzbarsky) → review+
Comment on attachment 8970749 [details]
Bug 1454694 Part 1: Apply the new FLAG_EVENTS_COMPLETE to progress notifications that are sent from complete or error states.

https://reviewboard.mozilla.org/r/239492/#review245506

What happened to the REQUSTED_REFLOW machinery?  The commit message should explain why it can go away....

Past that, I don't have reasonable confidence that I would be able to spot a problem here.  It's probably better to have someone more familiar with this code do this review.

::: image/imgRequest.cpp:942
(Diff revision 1)
>      // We have to fire the OnStopRequest notifications ourselves because there's
>      // no image capable of doing so.
>      Progress progress =
>        LoadCompleteProgress(lastPart, /* aError = */ false, status);
>  
> +    // Observers shouldn't expect to get more events from this request.

Do we no longer support multipart images?  At one point you could get multiple OnStopRequest for the same image, but maybe we took that out?
Attachment #8970749 - Flags: review?(bzbarsky)
Comment on attachment 8970654 [details]
Bug 1454694: Re-enable the timing-sensitive part of the CORS mode test.

https://reviewboard.mozilla.org/r/239398/#review245510
Attachment #8970654 - Flags: review?(bzbarsky) → review+
Comment on attachment 8970749 [details]
Bug 1454694 Part 1: Apply the new FLAG_EVENTS_COMPLETE to progress notifications that are sent from complete or error states.

https://reviewboard.mozilla.org/r/239492/#review245506

Yeah, my squashing of work got messy. I'll split out the changes that set the new flag from the changes that check the new flag.
Comment on attachment 8970749 [details]
Bug 1454694 Part 1: Apply the new FLAG_EVENTS_COMPLETE to progress notifications that are sent from complete or error states.

https://reviewboard.mozilla.org/r/239492/#review245534

::: image/imgRequest.cpp:942
(Diff revision 1)
>      // We have to fire the OnStopRequest notifications ourselves because there's
>      // no image capable of doing so.
>      Progress progress =
>        LoadCompleteProgress(lastPart, /* aError = */ false, status);
>  
> +    // Observers shouldn't expect to get more events from this request.

I didn't realize that. In that case, this is not an appropriate place to apply the new flag.
Attachment #8970749 - Flags: review?(bzbarsky)
Attachment #8971074 - Flags: review?(dbaron)
Comment on attachment 8970749 [details]
Bug 1454694 Part 1: Apply the new FLAG_EVENTS_COMPLETE to progress notifications that are sent from complete or error states.

Now that the flag-setting parts are separated from the flag-checking parts, can you re-review Part 1?
Attachment #8970749 - Flags: review?(bzbarsky)
Comment on attachment 8970749 [details]
Bug 1454694 Part 1: Apply the new FLAG_EVENTS_COMPLETE to progress notifications that are sent from complete or error states.

https://reviewboard.mozilla.org/r/239492/#review245584

Again, I'd really prefer that someone more familiar with imagelib review this.  Maybe Timothy?
Attachment #8970749 - Flags: review?(bzbarsky)
Attachment #8970749 - Flags: review?(tnikkel)
Blocks: 1457132
Comment on attachment 8971074 [details]
Bug 1454694 Part 2: Change ImageLoader to unblock onload in onLoadComplete whenever a request has STATUS_EVENTS_COMPLETE or STATUS_ERROR.

https://reviewboard.mozilla.org/r/239846/#review245942

I'm not sure I'm the best reviewer for this, but I do have some concerns that I think are worth answering before requesting review again (whether it's to me or somebody more familiar with the image code):

::: layout/style/ImageLoader.cpp:670
(Diff revision 1)
> +     status & (imgIRequest::STATUS_EVENTS_COMPLETE |
> +               imgIRequest::STATUS_ERROR)) {

Your comment seems to imply that you want to test if both bits are set (at least if I'm reading the comment correctly), but this tests whether just one of them is set.  Which is it that you want?

::: layout/style/ImageLoader.cpp:672
(Diff revision 1)
> +  // any more events, or has an error.
> +  uint32_t status = 0;
> +  if(NS_SUCCEEDED(aRequest->GetImageStatus(&status)) &&
> +     status & (imgIRequest::STATUS_EVENTS_COMPLETE |
> +               imgIRequest::STATUS_ERROR)) {
> -  // This is what happens in a CORS mode violation, and may happen during
> +    // This is what happens in a CORS mode violation, and may happen during

The "This" here was I think referring to the previous sentence "This may be called for ...", and seems like it no longer makes sense in the new context.
Attachment #8971074 - Flags: review?(dbaron)
Comment on attachment 8970749 [details]
Bug 1454694 Part 1: Apply the new FLAG_EVENTS_COMPLETE to progress notifications that are sent from complete or error states.

https://reviewboard.mozilla.org/r/239492/#review245972

Why do we need this? I read through all the commit msgs and comments in this bug and skimmed the patches and I couldn't see an explanation of what problem this is solving.
Attachment #8970749 - Flags: review?(tnikkel)
(In reply to Timothy Nikkel (:tnikkel) from comment #22)
> Comment on attachment 8970749 [details]
> Bug 1454694 Part 1: Apply the new FLAG_EVENTS_COMPLETE to progress
> notifications that are sent from complete or error states.
> 
> https://reviewboard.mozilla.org/r/239492/#review245972
> 
> Why do we need this? I read through all the commit msgs and comments in this
> bug and skimmed the patches and I couldn't see an explanation of what
> problem this is solving.

I'm sorry I haven't been clear on this. The only explanation is a too-subtle comment in Part 2. Here's the whole idea:

ImageLoader now blocks Document onload in response to some image loads. Specifically, images that are loaded with a flag of REQUEST_REQUIRES_REFLOW. This is done to support shape-outside: image usages where we don't want to fire onload until the first frame of the image is loaded and reflow has occurred. We also have to handle error conditions. In that case, we want to know when the error has occurred so we can unblock onload if necessary. That is done in ImageLoader::OnLoadComplete.

The problem occurs because some image requests send LOAD_COMPLETE before FRAME_COMPLETE, in cases where there is no error. In that case, our error catching code erroneously unblocks the document even though the frame will be later sent and reflow will complete correctly. This causes failing tests because onload is firing too early. It is the source of the intermittent in this bug and also in Bug 1454835 and Bug 1457132.

So this change adds a new status/event to image requests that means "this request is done firing events." ImageLoader::onLoadComplete checks if that flag is set, or if there is an error, and if either of those are true, the document is unblocked (if it was blocked on the image request in the first place).
(In reply to Brad Werth [:bradwerth] from comment #23)

> The problem occurs because some image requests send LOAD_COMPLETE before
> FRAME_COMPLETE, in cases where there is no error. In that case, our error
> catching code erroneously unblocks the document even though the frame will
> be later sent and reflow will complete correctly. This causes failing tests
> because onload is firing too early. It is the source of the intermittent in
> this bug and also in Bug 1454835 and Bug 1457132.

I'll open a new bug about this issue, and make this bug dependent on it. That way we can have a discussion there of this fact that LOAD_COMPLETE is not necessarily the last event sent from a request, and what ImageLoader will need to do to adjust to the event firing sequence. This bug will become blocked on that new bug. And Bug 1454835 and Bug 1457132 will become duplicates of the new bug. I don't want to rename any of the existing intermittent orange bugs because I think that might confuse the sherriffs during the period where this remains an issue.
Depends on: 1457532
No longer blocks: 1454835
Priority: -- → P3
Can this move forward now that the dependency has landed?
Flags: needinfo?(bwerth)
Attachment #8970653 - Attachment is obsolete: true
Attachment #8970749 - Attachment is obsolete: true
Attachment #8971074 - Attachment is obsolete: true
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3ab8e5d9cda5
Re-enable the timing-sensitive part of the CORS mode test. r=bz
https://hg.mozilla.org/mozilla-central/rev/3ab8e5d9cda5
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Flags: needinfo?(bwerth)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: