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

REOPENED
Assigned to

Status

()

defect
P3
normal
REOPENED
8 years ago
3 years ago

People

(Reporter: philor, Assigned: jwatt)

Tracking

({intermittent-failure})

Trunk
x86
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(b2g18 disabled, b2g18-v1.0.1 disabled)

Details

(Whiteboard: [test disabled][leave open])

Attachments

(1 attachment)

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.
Posted patch patchSplinter Review
Assignee: nobody → jwatt
Status: NEW → ASSIGNED
Attachment #592107 - Flags: review?(ehsan)
Attachment #592107 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/mozilla-central/rev/8df0708b2fe3
Status: ASSIGNED → RESOLVED
Closed: 8 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
Duplicate of this bug: 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