Closed Bug 1369350 Opened 7 years ago Closed 7 years ago

Intermittent layout/style/test/test_dont_use_document_colors.html | color is blocked - got "rgb(255, 255, 0)", expected "rgb(0, 0, 0)"

Categories

(Core :: CSS Parsing and Computation, defect, P4)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

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

References

Details

(Keywords: intermittent-failure, Whiteboard: [stockwell fixed:timing])

Attachments

(1 file, 1 obsolete file)

Add additional delay to test_dont_use_document_colors.html since nsPresContext delays applying prefs. r=dbaron
2.98 KB, patch
bkelly
: review+
Details | Diff | Splinter Review 
This doesn't seem stylo-only.  Could something have caused pushPrefEnv to not be reliable?
Priority: -- → P4
I've been doing some retriggers to isolate this; so far the earliest occurrence I got is on:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=4e8564ce92d8056d6282a499cac1933f4a90a769&filter-searchStr=stylo-seq+mochitest
but I suspect I'll get one earlier.
thanks for looking into this :dbaron.
Whiteboard: [stockwell needswork]
OK, I think this is a regression from bug 1363829, though it took an unusually large number of retriggers on the exact landing changeset to get a failure, relative to later changesets.

bkelly, any thoughts?
Blocks: 1363829
Flags: needinfo?(bkelly)
I'll take a look tomorrow, but my guess is that the issue is PushPrefEnv().  It uses a setTimeout(f, 0) to allow preferences to apply before calling its callback.  Speeding up setTimeout(f, 0) slightly may have caused this test to start losing a race there.
We're probably racing with this nsPresContext timer:

https://dxr.mozilla.org/mozilla-central/source/layout/base/nsPresContext.cpp#1549
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Flags: needinfo?(bkelly)
Might as well test a potential test fix as well:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c5dd13e704590ee0a36a228c1b90306560f5c61

Note, I'm leaning towards special casing this in the test vs trying to change SpecialPowers.pushPrefEnv().  It seems that this is due to nsPresShell's specific additional delay after preferences are changed and not a general preference thing.  (Assuming my theories all hold up.)
Comment on attachment 8875367 [details] [diff] [review]
Add additional delay to test_dont_use_document_colors.html since nsPresShell delays applying prefs. r=dbaron

This test is racing the setTimeout(f, 0) after built in to the pushPrefEnv() call against an nsPresContext specific nsITimer:

https://dxr.mozilla.org/mozilla-central/source/layout/base/nsPresContext.cpp#771

The nsPresContext is using a 0ms nsITimer which will round trip through the TimerThread.

The setTimeout(f, 0) can now result in a simple NS_DispatchToCurrentThread(r).

This patch makes the test wait for a setTimeout(f, 1) to account for the nsITimer in nsPresContext.  I think this is appropriate since the extra delay is only needed for nsPresContext preferences and not all preferences in general.
Attachment #8875367 - Flags: review?(dbaron)
Updated the comments and commit message to reference nsPresContext correctly.  I had previously written nsPresShell.
Attachment #8875367 - Attachment is obsolete: true
Attachment #8875699 - Flags: review+
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8c1a978f8fe
Add additional delay to test_dont_use_document_colors.html since nsPresContext delays applying prefs. r=dbaron
https://hg.mozilla.org/mozilla-central/rev/b8c1a978f8fe
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Whiteboard: [stockwell needswork] → [stockwell fixed:timing]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: