Closed
Bug 1317046
Opened 8 years ago
Closed 8 years ago
Intermittent TEST-UNEXPECTED-PASS | dom/base/test/browser_use_counters.js | page counts for PROPERTY_FILLOPACITY after are correct - Got 1, expected 1
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox51 | --- | unaffected |
firefox52 | --- | fixed |
firefox53 | --- | fixed |
People
(Reporter: intermittent-bug-filer, Assigned: froydnj)
Details
(Keywords: intermittent-failure)
Attachments
(1 file)
1.74 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
Filed by: archaeopteryx [at] coole-files.de https://treeherder.mozilla.org/logviewer.html#?job_id=39107061&repo=mozilla-inbound https://queue.taskcluster.net/v1/task/HVfJKhP6TdSUP1S4yVnP4A/runs/0/artifacts/public%2Flogs%2Flive_backing.log
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment 10•8 years ago
|
||
Nathan, this is our #3 intermittent orange over the last 3 days, I see the error as [1]: TEST-UNEXPECTED-PASS | dom/base/test/browser_use_counters.js | page counts for PROPERTY_FILLOPACITY after are correct - Got 1, expected 1 this is very frequent on linux64 debug and asan. Can you take a look at this and figure out what is going on? [1] https://treeherder.mozilla.org/logviewer.html#?repo=mozilla-inbound&job_id=39906828#L5960
Flags: needinfo?(nfroyd)
Assignee | ||
Comment 11•8 years ago
|
||
Feel free to redirect the review to someone else, but as you've reviewed the previous intermittent fix in this test...
Attachment #8815066 -
Flags: review?(dholbert)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → nfroyd
Flags: needinfo?(nfroyd)
Comment 12•8 years ago
|
||
Do you know *why* we'd be unexpectedly (and occasionally) passing this particular check? Presumably we had this "todo_is" here for a reason. If its assumptions are being violated, that seems maybe-worrisome. (Though maybe the "todo_is" is here for a bad reason, and really we just have no confidence about the value [correct or incorrect] when xfail=true?)
Flags: needinfo?(nfroyd)
Comment hidden (Intermittent Failures Robot) |
Assignee | ||
Comment 14•8 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #12) > Do you know *why* we'd be unexpectedly (and occasionally) passing this > particular check? > > Presumably we had this "todo_is" here for a reason. If its assumptions are > being violated, that seems maybe-worrisome. (Though maybe the "todo_is" is > here for a bad reason, and really we just have no confidence about the value > [correct or incorrect] when xfail=true?) check_use_counter_direct is used to test that SVGs loaded directly update their use counters. For the test where we pass xfail=true, we have an SVG document (call this A) with a fill pattern defined in a different SVG document (call this B). B is the document that touches the CSS property we're actually interested in for the test in question, and the histograms we're interested in will only get touched when B gets destroyed. We'd normally work around this race by eagerly updating the use counters for B from the test, but we don't have direct access to B, only to A--whose use counters we *do* eagerly update. When B updates the interesting histograms is then a question of when B gets destroyed. So these few UNEXPECTED-PASS results are happening because the race is working out not in our favor on these slower test machines (debug and asan). I don't think we have direct access to B from A for eagerly updating the use counters, which would be a better solution if it were possible.
Flags: needinfo?(nfroyd)
Comment 15•8 years ago
|
||
Thanks for explaining that! Perhaps we should just change our C++ code to make forced-use-counter-flushes for A implicitly also flush use counters for B? (Under the hood, the actual nsDocument for "A" here *does* have access to the nsDocument for "B". See e.g. nsDocument::FlushExternalResources and nsDocument::OnPageShow, each of which call "EnumerateExternalResources" to notifify all of the external-resource-docs like "B".) That doesn't seem too hard, and it'd allow the test to keep functioning as-expected, rather than skipping a bunch of testing in a particular case...
Comment hidden (Intermittent Failures Robot) |
Comment 17•8 years ago
|
||
ni for comment 15. (Let me know if you'd like me to take a crack at it, too - I didn't necessarily mean that you'd have to be the person who handles what I'm suggesting in comment 15. Though I think it should be pretty straightforward.)
Flags: needinfo?(nfroyd)
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment 22•8 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #12) > occasionally (In reply to Nathan Froyd [:froydnj] from comment #14) > few Currently our number 1 active failure, and based on some retriggering it fails 5-10% of the time on the more affected platforms, which is more than enough to justify disabling the entire test. I'd recommend taking a patch to disable a tiny part of the test, immediately, and only then worry about how to make it better.
Flags: needinfo?(dholbert)
Comment 23•8 years ago
|
||
Comment on attachment 8815066 [details] [diff] [review] avoid using todo_is in browser_use_counters.js Review of attachment 8815066 [details] [diff] [review]: ----------------------------------------------------------------- Fair enough. Could you file a followup on comment 15, though, to fix this correctly? In the meantime, I suppose this seems fine.
Attachment #8815066 -
Flags: review?(dholbert) → review+
Comment 24•8 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #23) > Fair enough. Could you file a followup on comment 15, though, to fix this > correctly? ("you" there is directed at froydnj.)
Flags: needinfo?(dholbert)
Comment 25•8 years ago
|
||
Pushed by nfroyd@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/590c70d2662a avoid using todo_is in browser_use_counters.js; r=dholbert
Assignee | ||
Comment 26•8 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #23) > Fair enough. Could you file a followup on comment 15, though, to fix this > correctly? ni? myself to make sure I do this on Tuesday. > In the meantime, I suppose this seems fine. Thank you!
Flags: needinfo?(nfroyd)
Comment 28•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/590c70d2662a
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Assignee | ||
Comment 29•8 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #23) > Fair enough. Could you file a followup on comment 15, though, to fix this > correctly? Filed bug 1322396.
Flags: needinfo?(nfroyd)
Updated•7 years ago
|
status-firefox51:
--- → unaffected
status-firefox52:
--- → affected
Comment 30•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/cf05a9412d74
Flags: in-testsuite+
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•