Closed Bug 1451196 Opened 4 years ago Closed 4 years ago

Intermittent layout/style/test/test_shape_outside_CORS.html | Test 1: Sibling is at same left offset as div (shape-outside was allowed).

Categories

(Core :: DOM: CSS Object Model, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: bradwerth)

References

Details

(Keywords: intermittent-failure, Whiteboard: [retriggered][stockwell disabled])

Attachments

(1 file)

Filed by: apavel [at] mozilla.com

https://treeherder.mozilla.org/logviewer.html#?job_id=171761456&repo=mozilla-inbound

https://queue.taskcluster.net/v1/task/WIOly6pYSYeq94MqjAavWA/runs/0/artifacts/public/logs/live_backing.log

[task 2018-04-04T01:03:51.094Z] 01:03:51     INFO - TEST-START | layout/style/test/test_shape_outside_CORS.html
[task 2018-04-04T01:03:51.357Z] 01:03:51     INFO - TEST-INFO | started process screentopng
[task 2018-04-04T01:03:51.962Z] 01:03:51     INFO - TEST-INFO | screentopng: exit 0
[task 2018-04-04T01:03:51.964Z] 01:03:51     INFO - TEST-UNEXPECTED-FAIL | layout/style/test/test_shape_outside_CORS.html | Test 1: Sibling is at same left offset as div (shape-outside was allowed). 
[task 2018-04-04T01:03:51.965Z] 01:03:51     INFO -     receiveMessage@layout/style/test/test_shape_outside_CORS.html:26:3
[task 2018-04-04T01:03:51.966Z] 01:03:51     INFO -     EventListener.handleEvent*runTests@layout/style/test/test_shape_outside_CORS.html:31:3
[task 2018-04-04T01:03:51.967Z] 01:03:51     INFO -     onload@layout/style/test/test_shape_outside_CORS.html:1:1
[task 2018-04-04T01:03:51.969Z] 01:03:51     INFO - TEST-PASS | layout/style/test/test_shape_outside_CORS.html | Test 2: Sibling is at different left offset from div (shape-outside was refused). 
[task 2018-04-04T01:03:51.970Z] 01:03:51     INFO - GECKO(1815) | MEMORY STAT | vsize 840MB | residentFast 239MB | heapAllocated 36MB
[task 2018-04-04T01:03:51.971Z] 01:03:51     INFO - TEST-OK | layout/style/test/test_shape_outside_CORS.html | took 288ms
Summary: Intermittent layout/style/tes/test_shape_outside_CORS.htmlt | Test 1: Sibling is at same left offset as div (shape-outside was allowed). → Intermittent layout/style/test/test_shape_outside_CORS.html | Test 1: Sibling is at same left offset as div (shape-outside was allowed).
Hi! I did some retriggers on this test to see if I can find the culprit and it seems to have started with this push. From 20 retriggers it failed 3 times. 

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-searchStr=opt-mochitest-e10s-5&fromchange=c93a8362c8ac026f28a27f5dc88c5a4cac071a9b&tochange=d66f4a7d60343c2f9ea8278f5bc35efcad79bb7c&selectedJob=171999693

The first actual test fail was 6 pushes later when the bug was filled. 
On the push that started it was worked only on Bug 1404222: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=917f8c2a17daf42b40be516605cd0b4ed893b1d1&filter-searchStr=opt-mochitest-e10s-5&selectedJob=171999693

Daniel could you please take a look at this? Thank you.
Flags: needinfo?(jmaher)
Flags: needinfo?(dholbert)
Whiteboard: [stockwell needswork][retriggered]
[redirecting needinfo to Brad]
Flags: needinfo?(dholbert) → needinfo?(bwerth)
Blocks: 1404222
Depends on: 1418930
Thanks Cosmin- this looks to be the right root cause- I assume Brad will be able to fix this up!
Flags: needinfo?(jmaher)
Assignee: nobody → bwerth
Attachment #8965468 - Flags: review?(dholbert)
(In reply to Brad Werth [:bradwerth] from comment #6)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=21e50f654978b4670a1f99baca0c09857bac77e0

The patch seems to eliminate the intermittent. I'm running the test-linux32/opt-mochitest-e10s-5 job a few more times (3 successes so far) to confirm. The code is ready for review.
Attachment #8965468 - Flags: review?(dholbert)
Priority: P5 → P1
The patch has generated 20 straight successes in the try run and I'm testing 20 more times to make sure.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2c85ca1a1518023e7c365487f3e6cb1b0ece329&selectedJob=172396621
Attachment #8965468 - Flags: review?(dholbert)
Comment on attachment 8965468 [details]
Bug 1451196: Disable the intermittent part of test layout/style/test/test_shape_outside_CORS.html.

https://reviewboard.mozilla.org/r/234222/#review240756

::: layout/style/ImageLoader.cpp:208
(Diff revision 2)
> +        // a new frame to the same request shortly after. This is a pattern
> +        // in nsCSSFrameConstructor::RecreateFramesForContent, for example.
> +        // Because we don't want to momentarily unblock onload just to block
> +        // it again, we put the call to unblock onload on a dispatch.
> +        // We pass true to UnblockOnload since the call is already async.
> +        NS_IdleDispatchToCurrentThread(

I'm not sure "Idle Dispatch" is really what we want here....  I'm not super-familiar with this API & the idle event queue, but my impression is that it's treated as low-priority and could get stuck behind other work. (It'll run eventually, but maybe not for a little while if we're backlogged.)

And I don't know that that's really what you want here -- really, we'd like to unblock onload as soon as possible (after we're confident we'll be out of the RecreateFramesForContent call that you're referencing in the comment, for example).

So if RecreateFramesForContent is all we're worried about here, then I'd expect NS_DispatchToCurrentThread should work (no idle).  If it doesn't, then there's something more subtle going on, I think; and I imagine the idle version isn't really "fixing" that subtle thing, but rather doing the equivalent of setTimeout(callbackFunc, $arbitrary-delay); ...
Attachment #8965468 - Flags: review?(dholbert) → review-
Also: I understand why using async-dispatch for "UnblockOnload" would be useful here, but I don't immediately understand how it corresponds to the test failure ("Sibling is at same left offset as div (shape-outside was allowed).")

Are we really allowing shape-outside:<cross-origin-image> when we should not, as the test's output seems to be saying?  Or are we failing in another way that happens to make this comparison fail for unrelated-to-CORS reasons?  I'm guessing it's the latter, based on the patch, which means we perhaps should adjust the wording in the test (and/or make the comparison more specific/subtle).
er - RE the test failure message, maybe I'm just misunderstanding the message.  Maybe it really means to say "shape-outside *should* be allowed" [but wasn't]?

If so: that sounds less like a worrisome CORS violation, which is good, but then this wording definitely needs fixing.   Note that mochitest/WPT-test messages really are really only looked at by humans when there's a test failure -- so they need to be written such that they can be unambiguously interpreted in the context of TEST-UNEXPECTED-FAIL [message]".  The best way to do that is to phrase them with expectation-flavored language ("should", etc.), rather than language that states what did/didn't happen.
(In reply to Daniel Holbert [:dholbert] (away 4/24 - 5/11) from comment #17)
> er - RE the test failure message, maybe I'm just misunderstanding the
> message.  Maybe it really means to say "shape-outside *should* be allowed"
> [but wasn't]?

Yes, the messages are failing to say "should" when they do indeed mean that. The successful but flawed patch indicates that in this case the error message does not indicate a failure to block a bad CORS request. The failure indicated in the logs is a failure to reflow after fetching the CORS mode image successfully. I'll update the test to make clear that either issue could trigger test failure. I can't think of a way to write the test that isolates those two possibilities.

(In reply to Daniel Holbert [:dholbert] (away 4/24 - 5/11) from comment #15)
> So if RecreateFramesForContent is all we're worried about here, then I'd
> expect NS_DispatchToCurrentThread should work (no idle).  If it doesn't,
> then there's something more subtle going on, I think; and I imagine the idle
> version isn't really "fixing" that subtle thing, but rather doing the
> equivalent of setTimeout(callbackFunc, $arbitrary-delay); ...

You must be correct, but I don't know what else could be going on. I'm going to rework the patch to put a blockunload/unblockonload pair around all the possible calling sites that could call RemoveRequestToFrameMapping before a call to AssociateRequestToFrame on the same request. nsCSSFrameConstructor::RecreateFramesForContent is one case; DidSetStyleContext is another.
(In reply to Brad Werth [:bradwerth] from comment #18)
> I'll update the test to make clear that either issue
> could trigger test failure. I can't think of a way to write the test that
> isolates those two possibilities.

Thanks! Yeah, fixing the message seems sufficient.

> (In reply to Daniel Holbert [:dholbert] (away 4/24 - 5/11) from comment #15)
> You must be correct, but I don't know what else could be going on. I'm going
> to rework the patch to put a blockunload/unblockonload pair around all the
> possible calling sites that could call RemoveRequestToFrameMapping before a
> call to AssociateRequestToFrame on the same request.
> nsCSSFrameConstructor::RecreateFramesForContent is one case;
> DidSetStyleContext is another.

That seems like an interesting thing to run through Try to further diagnose things here.  But I'm hoping we can get by with something less invasive than that...  And conceptually, that seems like it might make us unblock *sooner* than your initial NS_DispatchToCurrentThread strategy would, so I'm not sure it'd help.

It might be better to try to add some logging to BlockOnload, UnblockOnload, RemoveRequestToFrameMapping, and AssociateRequestToFrame (along with whatever part of UnblockOnload decides that "ok it's time to fire onload") -- and then run that through Try with your original non-idle NS_DispatchToCurrentThread patch here.  That might help isolate what's going on with that patch.
Comment on attachment 8965468 [details]
Bug 1451196: Disable the intermittent part of test layout/style/test/test_shape_outside_CORS.html.

https://reviewboard.mozilla.org/r/234222/#review242092

::: layout/style/ImageLoader.cpp:208
(Diff revision 2)
> +        NS_IdleDispatchToCurrentThread(
> +          NewRunnableMethod<bool>("nsIDocument::UnblockOnload",
> +                                  mDocument,
> +                                  &nsIDocument::UnblockOnload,
> +                                  true));

One more issue with the patch as-is: if we do end up dispatching this as a runnable, that runnable needs to hold a strong reference to mDocument. (Which means it might need to be slightly fancier than NewRunnableMethod<>(...) -- you might need to define a small runnable subclass.)

With the patch right now, I'm not sure we have any guarantee that mDocument is still alive when this runnable is fired, so it could result in a use-after-free.
Brad, could you please have another try on fixing this bug? It's failing a lot in the last couple of days. Or maybe make a patch for disabling it, if it's possible, and then come back at it. Thank you.
Comment on attachment 8965468 [details]
Bug 1451196: Disable the intermittent part of test layout/style/test/test_shape_outside_CORS.html.

https://reviewboard.mozilla.org/r/234222/#review243076


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: layout/style/test/mochitest.ini:302
(Diff revision 3)
>  [test_rules_out_of_sheets.html]
>  [test_selectors.html]
>  skip-if = toolkit == 'android' #bug 775227
>  [test_selectors_on_anonymous_content.html]
>  [test_setPropertyWithNull.html]
> -[test_shape_outside_CORS.html]
> +#[test_shape_outside_CORS.html]

Error: "use 'disabled=<reason>' to disable a test instead of a comment"
 [no-comment-disable]
typically we do:
[test_shape_outside_CORS.html]
skip-if = true # Bug 1451196
(In reply to Joel Maher ( :jmaher) (UTC-5) from comment #31)
> typically we do:
> [test_shape_outside_CORS.html]
> skip-if = true # Bug 1451196

Thanks. Daniel Holbert pointed out that I could just disable the first part of the test, instead of disabling the whole test. Since the second part of the test actually verifies that we block loads via CORS, it's the more important part anyway. I've done that in the latest patch.
Comment on attachment 8965468 [details]
Bug 1451196: Disable the intermittent part of test layout/style/test/test_shape_outside_CORS.html.

https://reviewboard.mozilla.org/r/234222/#review243084

LGTM, thanks. Let's try to fix bug 1454694 before this hits release. (Seems like a good bet that the fix will be relatively safe to uplift to beta).
Attachment #8965468 - Flags: review?(dholbert) → review+
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bdc39d29d014
Disable the intermittent part of test layout/style/test/test_shape_outside_CORS.html. r=dholbert
https://hg.mozilla.org/mozilla-central/rev/bdc39d29d014
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Flags: needinfo?(bwerth)
Whiteboard: [retriggered][stockwell disable-recommended] → [retriggered][stockwell disabled]
You need to log in before you can comment on or make changes to this bug.