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)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox51 --- unaffected
firefox52 --- fixed
firefox53 --- fixed

People

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

Details

(Keywords: intermittent-failure)

Attachments

(1 file)

avoid using todo_is in browser_use_counters.js
1.74 KB, patch
dholbert
: review+
Details | Diff | Splinter Review 
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)
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: nobody → nfroyd
Flags: needinfo?(nfroyd)
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)
(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)
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...
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)
(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 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+
(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)
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
(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)
Try that again. :(
Flags: needinfo?(nfroyd)
https://hg.mozilla.org/mozilla-central/rev/590c70d2662a
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(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)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: