Closed
Bug 1451196
Opened 6 years ago
Closed 6 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)
Core
DOM: CSS Object Model
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
Updated•6 years ago
|
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).
Comment hidden (Intermittent Failures Robot) |
Comment 2•6 years ago
|
||
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]
Comment 3•6 years ago
|
||
[redirecting needinfo to Brad]
Flags: needinfo?(dholbert) → needinfo?(bwerth)
Updated•6 years ago
|
Comment 4•6 years ago
|
||
Thanks Cosmin- this looks to be the right root cause- I assume Brad will be able to fix this up!
Flags: needinfo?(jmaher)
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → bwerth
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=21e50f654978b4670a1f99baca0c09857bac77e0
Comment hidden (Intermittent Failures Robot) |
Assignee | ||
Updated•6 years ago
|
Attachment #8965468 -
Flags: review?(dholbert)
Assignee | ||
Comment 8•6 years ago
|
||
(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.
Assignee | ||
Comment 9•6 years ago
|
||
Whoops, failed on the 9th try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=21e50f654978b4670a1f99baca0c09857bac77e0
Updated•6 years ago
|
Attachment #8965468 -
Flags: review?(dholbert)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2c85ca1a1518023e7c365487f3e6cb1b0ece329
Comment hidden (Intermittent Failures Robot) |
Priority: P5 → P1
Comment hidden (Intermittent Failures Robot) |
Assignee | ||
Comment 14•6 years ago
|
||
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
Assignee | ||
Updated•6 years ago
|
Attachment #8965468 -
Flags: review?(dholbert)
Comment 15•6 years ago
|
||
mozreview-review |
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-
Comment 16•6 years ago
|
||
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).
Comment 17•6 years ago
|
||
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.
Assignee | ||
Comment 18•6 years ago
|
||
(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.
Comment 19•6 years ago
|
||
(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 hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment 24•6 years ago
|
||
mozreview-review |
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.
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment 28•6 years ago
|
||
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 hidden (mozreview-request) |
Comment 30•6 years ago
|
||
mozreview-review |
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]
Comment 31•6 years ago
|
||
typically we do: [test_shape_outside_CORS.html] skip-if = true # Bug 1451196
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 34•6 years ago
|
||
(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.
Assignee | ||
Comment 35•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ccb8da338681f1d3f27e2f1f1751c8abaad3bac2
Comment 36•6 years ago
|
||
mozreview-review |
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+
Comment 37•6 years ago
|
||
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
Comment 38•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bdc39d29d014
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(bwerth)
Comment hidden (Intermittent Failures Robot) |
Updated•6 years ago
|
Whiteboard: [retriggered][stockwell disable-recommended] → [retriggered][stockwell disabled]
Comment hidden (Intermittent Failures Robot) |
You need to log in
before you can comment on or make changes to this bug.
Description
•