Open Bug 701060 Opened 13 years ago Updated 2 years ago

Intermittent test_bug401046.html, test_lang.xhtml | at min font size 18, 4px should compute to 18px - got 7px, expected 18px and | at min font size 18, 12px should compute to 18px - got 12px, expected 18px

Categories

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

x86
Windows 7
defect

Tracking

()

REOPENED
Tracking Status
b2g18 --- disabled
b2g18-v1.0.1 --- disabled

People

(Reporter: philor, Assigned: jwatt)

References

Details

(Keywords: intermittent-failure, Whiteboard: [test disabled][leave open])

Attachments

(1 file)

https://tbpl.mozilla.org/php/getParsedLog.php?id=7303269&tree=Mozilla-Inbound Rev3 WINNT 6.1 mozilla-inbound pgo test mochitests-4/5 on 2011-11-09 02:31:43 PST for push c3f33adc6080 6965 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/style/test/test_bug401046.html | at min font size 18, 4px should compute to 18px - got 7px, expected 18px 6966 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/style/test/test_bug401046.html | at min font size 18, 12px should compute to 18px - got 12px, expected 18px
It seems to me that SpecialPowers.pushPrefEnv() waits until the pref system has consumed the new value before calling its callback, but not until the pref system observers have completed. In other words, in the case of this testcase, not until all documents have recomputed their default styles based on the new pref values. Certainly the failures here are all consistent with that hypothesis. I'm not sure exactly how we can make sure we wait until the document is restyled for the pref change. Ideally SpecialPowers.pushPrefEnv() would only call its callback after all observers of the pref completed. If that's not easy, then perhaps we can listen for a restyle event (if there is such a thing).
Ehsan, David, see the previous comment.
(In reply to Jonathan Watt [:jwatt] from comment #21) > It seems to me that SpecialPowers.pushPrefEnv() waits until the pref system > has consumed the new value before calling its callback, but not until the > pref system observers have completed. In other words, in the case of this > testcase, not until all documents have recomputed their default styles based > on the new pref values. Certainly the failures here are all consistent with > that hypothesis. > > I'm not sure exactly how we can make sure we wait until the document is > restyled for the pref change. Ideally SpecialPowers.pushPrefEnv() would only > call its callback after all observers of the pref completed. If that's not > easy, then perhaps we can listen for a restyle event (if there is such a > thing). This seems like a good theory. The pref service calls all of the observers without returning to the event loop, so simply calling SpecialPowers.executeSoon(callback) instead of callback itself will make sure that the callback will be called after all other observers have finished running. Would that be enough to fix the problem here?
(In reply to Ehsan Akhgari [:ehsan] from comment #25) > This seems like a good theory. The pref service calls all of the observers > without returning to the event loop, so simply calling > SpecialPowers.executeSoon(callback) instead of callback itself will make > sure that the callback will be called after all other observers have > finished running. Would that be enough to fix the problem here? So are you saying that currently the callback is called without returning to the event loop? If that's true, then I have to wonder how these tests ever pass, since surely then there would never be any opportunity for the document to be restyled. Sounds like executeSoon is still worth a shot though. I can land a patch for that a bit later if you don't get to it first.
(In reply to Jonathan Watt [:jwatt] from comment #26) > (In reply to Ehsan Akhgari [:ehsan] from comment #25) > > This seems like a good theory. The pref service calls all of the observers > > without returning to the event loop, so simply calling > > SpecialPowers.executeSoon(callback) instead of callback itself will make > > sure that the callback will be called after all other observers have > > finished running. Would that be enough to fix the problem here? > > So are you saying that currently the callback is called without returning to > the event loop? If that's true, then I have to wonder how these tests ever > pass, since surely then there would never be any opportunity for the > document to be restyled. The callback is called from an observer for a pref. The preferences service calls all of its observers in a loop without returning to the event loop in an order which you cannot rely on. This means that it is possible for callback to not be the last observer which is called, which means that it may run before something that you rely on has been executed. > Sounds like executeSoon is still worth a shot though. I can land a patch for > that a bit later if you don't get to it first. I'm sure you will beat me to it, but I will happily review the patch if you need me to. :-)
(In reply to Ehsan Akhgari [:ehsan] from comment #27) > The callback is called from an observer for a pref. The preferences service > calls all of its observers in a loop without returning to the event loop in > an order which you cannot rely on. This means that it is possible for > callback to not be the last observer which is called, which means that it > may run before something that you rely on has been executed. Okay, I see, so the pref observer for the nsPresShell and the observers for the callbacks in the test get called together, but in an undefined order. Also, note that _both_ delay their work by posting an event to the event queue. (The nsPresContext observer posts an event to call PrefChangedUpdateTimerCallback to do the restyle. The callbacks in test_bug401046.html use setTimeout to delay doing their checks.) Of course since the order in which those events are processed depends on the order in which they're posted, the race condition still exists. But yeah, in light of that, it makes perfect sense that delaying the test's checks by making it go via the event queue for a _second_ time would solve the problem. In fact given that pref observers in the mozilla code not infrequently defer their work using the event loop, it probably makes sense to put a delay into pushPrefEnv to account for that too.
Attached patch patch — — Splinter Review
Assignee: nobody → jwatt
Status: NEW → ASSIGNED
Attachment #592107 - Flags: review?(ehsan)
Attachment #592107 - Flags: review?(ehsan) → review+
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
Clearly for some reason that patch wasn't enough.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: 731911
Summary: Intermittent test_bug401046.html | at min font size 18, 4px should compute to 18px - got 7px, expected 18px and | | at min font size 18, 12px should compute to 18px - got 12px, expected 18px → Intermittent test_bug401046.html | at min font size 18, 4px should compute to 18px - got 7px, expected 18px and | at min font size 18, 12px should compute to 18px - got 12px, expected 18px
Target Milestone: mozilla12 → ---
Merging bug 731911 (and its 70 failures) into this, per 731911 comment 1 :-)
Summary: Intermittent test_bug401046.html | at min font size 18, 4px should compute to 18px - got 7px, expected 18px and | at min font size 18, 12px should compute to 18px - got 12px, expected 18px → Intermittent test_bug401046.html, test_buffered.html | at min font size 18, 4px should compute to 18px - got 7px, expected 18px and | at min font size 18, 12px should compute to 18px - got 12px, expected 18px
No longer blocks: 731911
Rev3 WINNT 5.1 mozilla-inbound opt test mochitests-1/5 on 2012-08-05 06:21:28 PDT for push ab9cc855bb39 slave: talos-r3-xp-019 https://tbpl.mozilla.org/php/getParsedLog.php?id=14143021&tree=Mozilla-Inbound 123231 ERROR TEST-UNEXPECTED-FAIL | /tests/content/svg/content/test/test_lang.xhtml | at min font size 18, 4px should compute to 18px - got 7px, expected 18px 123232 ERROR TEST-UNEXPECTED-FAIL | /tests/content/svg/content/test/test_lang.xhtml | at min font size 18, 12px should compute to 18px - got 12px, expected 18px
Summary: Intermittent test_bug401046.html, test_buffered.html | at min font size 18, 4px should compute to 18px - got 7px, expected 18px and | at min font size 18, 12px should compute to 18px - got 12px, expected 18px → Intermittent test_bug401046.html, test_buffered.html, test_lang.xhtml | at min font size 18, 4px should compute to 18px - got 7px, expected 18px and | at min font size 18, 12px should compute to 18px - got 12px, expected 18px
Toporange with several hundred failures not telling us anything new. Disabled for now as part of the drive to reduce our OrangeFactor: https://hg.mozilla.org/integration/mozilla-inbound/rev/bbdab9ec44a1
Whiteboard: [orange] → [orange][test disabled][leave open]
Summary: Intermittent test_bug401046.html, test_buffered.html, test_lang.xhtml | at min font size 18, 4px should compute to 18px - got 7px, expected 18px and | at min font size 18, 12px should compute to 18px - got 12px, expected 18px → Intermittent test_bug401046.html, test_lang.xhtml | at min font size 18, 4px should compute to 18px - got 7px, expected 18px and | at min font size 18, 12px should compute to 18px - got 12px, expected 18px
Whiteboard: [orange][test disabled][leave open] → [test disabled][leave open]
Attachment #592107 - Flags: checkin+
Bulk assigning P3 to all open intermittent bugs without a priority set in Firefox components per bug 1298978.
Priority: -- → P3
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: