We currently update nsRefreshDriver::mMostRecentRefresh in nsRefreshDriver::EnsureTimerStarted[1]. [1] https://dxr.mozilla.org/mozilla-central/rev/388bdc46ba51ee31da8b8abe977e0ca38d117434/layout/base/nsRefreshDriver.cpp#1195 But in case of EnsureTimerStarted is called from nsRefreshDriver::ScheduleViewManagerFlush we don't need to update mMostRecentRefresh otherwise the painting process caused by ScheduleViewManagerFlush won't be done in the same frame. This issue was found by a test case in bug 1235286 comment #0 (No.7).

EnsureTimerStarted is called from various places without arguments. I am confident that　ScheduleViewManagerFlush does not need to update mMostRecentRefresh but I am not sure other callers too. To preserve the behavior of EnsureTimerStarted from the other callers I'd propose here to add a new flag names eNeverAdjustTimer which represents that EnsureTimerStarted does not update mMostRecentRefresh. It's only for ScheduleViewManagerFlush. I'd also propose to rename existing eAdjustingTimer to eForceAdjustTimer for avoiding confusion.

Review commit: https://reviewboard.mozilla.org/r/29189/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/29189/

Review commit: https://reviewboard.mozilla.org/r/29191/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/29191/

(In reply to Hiroyuki Ikezoe (:hiro) from comment #0) > We currently update nsRefreshDriver::mMostRecentRefresh in > nsRefreshDriver::EnsureTimerStarted[1]. > > [1] > https://dxr.mozilla.org/mozilla-central/rev/ > 388bdc46ba51ee31da8b8abe977e0ca38d117434/layout/base/nsRefreshDriver.cpp#1195 > > But in case of EnsureTimerStarted is called from > nsRefreshDriver::ScheduleViewManagerFlush we don't need to update > mMostRecentRefresh otherwise the painting process caused by > ScheduleViewManagerFlush won't be done in the same frame. > > This issue was found by a test case in bug 1235286 comment #0 (No.7). I'm kind of confused as to what this is supposed to be doing. EnsureTimerStarted() should only be making sure that we start at timer, then when a vsync occurs, we paint. This should update the mMostRecentRefresh time during the tick [1]. Why are there two frames occurring instead? Are we force flushing in addition to scheduling a flush? Otherwise, it looks like ScheduleViewManagerFlush() should still wait for a vsync to flush. [1] https://dxr.mozilla.org/mozilla-central/rev/388bdc46ba51ee31da8b8abe977e0ca38d117434/layout/base/nsRefreshDriver.cpp#1612

(In reply to Mason Chang [:mchang] from comment #5) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #0) > > We currently update nsRefreshDriver::mMostRecentRefresh in > > nsRefreshDriver::EnsureTimerStarted[1]. > > > > [1] > > https://dxr.mozilla.org/mozilla-central/rev/ > > 388bdc46ba51ee31da8b8abe977e0ca38d117434/layout/base/nsRefreshDriver.cpp#1195 > > > > But in case of EnsureTimerStarted is called from > > nsRefreshDriver::ScheduleViewManagerFlush we don't need to update > > mMostRecentRefresh otherwise the painting process caused by > > ScheduleViewManagerFlush won't be done in the same frame. > > > > This issue was found by a test case in bug 1235286 comment #0 (No.7). > > I'm kind of confused as to what this is supposed to be doing. > EnsureTimerStarted() should only be making sure that we start at timer, then > when a vsync occurs, we paint. This should update the mMostRecentRefresh > time during the tick [1]. Why are there two frames occurring instead? For example on non-E10S, consider a style for a element is updated in requestAnimationFrame. 1. content refresh driver's Tick is called. 2. the requestAnimationFrame is called. 3. the style is updated. 4. root refresh driver's ScheduleViewManagerFlush is called. 5. root refresh driver's EnsureTimerStarted is called. 6. root refresh driver's mMostRecentRefresh is updated to the same time as content refresh driver's. 7. root refresh driver's Tick is called. But... 8. skip the process of the root refresh driver's Tick because mMostRecentRefresh equals already to aNowTime. 9. mViewManagerFlushIsPending is still true. Then... 10. painting process will be done in the next tick.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #6) > (In reply to Mason Chang [:mchang] from comment #5) > > (In reply to Hiroyuki Ikezoe (:hiro) from comment #0) > > > We currently update nsRefreshDriver::mMostRecentRefresh in > > > nsRefreshDriver::EnsureTimerStarted[1]. > > > > > > [1] > > > https://dxr.mozilla.org/mozilla-central/rev/ > > > 388bdc46ba51ee31da8b8abe977e0ca38d117434/layout/base/nsRefreshDriver.cpp#1195 > > > > > > But in case of EnsureTimerStarted is called from > > > nsRefreshDriver::ScheduleViewManagerFlush we don't need to update > > > mMostRecentRefresh otherwise the painting process caused by > > > ScheduleViewManagerFlush won't be done in the same frame. > > > > > > This issue was found by a test case in bug 1235286 comment #0 (No.7). > > > > I'm kind of confused as to what this is supposed to be doing. > > EnsureTimerStarted() should only be making sure that we start at timer, then > > when a vsync occurs, we paint. This should update the mMostRecentRefresh > > time during the tick [1]. Why are there two frames occurring instead? > > For example on non-E10S, consider a style for a element is updated in > requestAnimationFrame. > > 1. content refresh driver's Tick is called. > 2. the requestAnimationFrame is called. > 3. the style is updated. > 4. root refresh driver's ScheduleViewManagerFlush is called. > 5. root refresh driver's EnsureTimerStarted is called. > 6. root refresh driver's mMostRecentRefresh is updated to the same time as > content refresh driver's. Shouldn't this prevent this from happening - https://dxr.mozilla.org/mozilla-central/source/layout/base/nsRefreshDriver.cpp?case=true&from=EnsureTimerStarted#1155 The default should be to be passing in no flags and thus that return should occur if it's ticking off an active timer already?. > 7. root refresh driver's Tick is called. But... > 8. skip the process of the root refresh driver's Tick because > mMostRecentRefresh equals already to aNowTime. > 9. mViewManagerFlushIsPending is still true. Then... > 10. painting process will be done in the next tick. Thanks for breaking it down!

(In reply to Mason Chang [:mchang] from comment #7) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #6) > > (In reply to Mason Chang [:mchang] from comment #5) > > > (In reply to Hiroyuki Ikezoe (:hiro) from comment #0) > > > > We currently update nsRefreshDriver::mMostRecentRefresh in > > > > nsRefreshDriver::EnsureTimerStarted[1]. > > > > > > > > [1] > > > > https://dxr.mozilla.org/mozilla-central/rev/ > > > > 388bdc46ba51ee31da8b8abe977e0ca38d117434/layout/base/nsRefreshDriver.cpp#1195 > > > > > > > > But in case of EnsureTimerStarted is called from > > > > nsRefreshDriver::ScheduleViewManagerFlush we don't need to update > > > > mMostRecentRefresh otherwise the painting process caused by > > > > ScheduleViewManagerFlush won't be done in the same frame. > > > > > > > > This issue was found by a test case in bug 1235286 comment #0 (No.7). > > > > > > I'm kind of confused as to what this is supposed to be doing. > > > EnsureTimerStarted() should only be making sure that we start at timer, then > > > when a vsync occurs, we paint. This should update the mMostRecentRefresh > > > time during the tick [1]. Why are there two frames occurring instead? > > > > For example on non-E10S, consider a style for a element is updated in > > requestAnimationFrame. > > > > 1. content refresh driver's Tick is called. > > 2. the requestAnimationFrame is called. > > 3. the style is updated. > > 4. root refresh driver's ScheduleViewManagerFlush is called. > > 5. root refresh driver's EnsureTimerStarted is called. > > 6. root refresh driver's mMostRecentRefresh is updated to the same time as > > content refresh driver's. > > Shouldn't this prevent this from happening - > https://dxr.mozilla.org/mozilla-central/source/layout/base/nsRefreshDriver. > cpp?case=true&from=EnsureTimerStarted#1155 > > The default should be to be passing in no flags and thus that return should > occur if it's ticking off an active timer already?. Oops, I am sorry. I forgot to say an important precondition. That is the root refresh driver is dormant before the content refresh driver's tick. It's very common case on E10S.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #8) > > Shouldn't this prevent this from happening - > > https://dxr.mozilla.org/mozilla-central/source/layout/base/nsRefreshDriver. > > cpp?case=true&from=EnsureTimerStarted#1155 > > > > The default should be to be passing in no flags and thus that return should > > occur if it's ticking off an active timer already?. > > Oops, I am sorry. I forgot to say an important precondition. That is the > root refresh driver is dormant before the content refresh driver's tick. > It's very common case on E10S. I meant non-E10S, of course. But it's common regardless of E10S/non-E10S.

From comment 8, does that mean this can only happen on the first root refresh driver's tick? As in, once the root refresh driver has started to tick as well, the mMostRecentRefresh won't be updated and this problem does not occur?

(In reply to Mason Chang [:mchang] from comment #10) > From comment 8, does that mean this can only happen on the first root > refresh driver's tick? No. Sorry for the confusion. 'dormant' means inactive. If the root refresh driver is active, this problem does not occur.

Comment on attachment 8702806 [details] MozReview Request: Bug 1235478 - Part 1: Rename eAdjustingTimer to eForceAdjustTimer. r=mchang https://reviewboard.mozilla.org/r/29189/#review26313

Comment on attachment 8702807 [details] MozReview Request: Bug 1235478 - Part 2: Don't update mMostRecentRefresh when nsRefreshDriver::ScheduleViewManagerFlush is called. r=mchang https://reviewboard.mozilla.org/r/29191/#review26315 ::: layout/base/nsRefreshDriver.cpp:1187 (Diff revision 1) > + if (aFlags & eNeverAdjustTimer) { Please put a comment here explaining why this happens. You can put something like "When switching from an inactive timer to an active timer, the root refresh driver is skipped due to being set to the content refresh driver's timestamp".

Comment on attachment 8702806 [details] MozReview Request: Bug 1235478 - Part 1: Rename eAdjustingTimer to eForceAdjustTimer. r=mchang Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29189/diff/1-2/

Comment on attachment 8702807 [details] MozReview Request: Bug 1235478 - Part 2: Don't update mMostRecentRefresh when nsRefreshDriver::ScheduleViewManagerFlush is called. r=mchang Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29191/diff/1-2/

Thank you, Mason! https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9872e6fac6f

https://hg.mozilla.org/integration/mozilla-inbound/rev/02c3c24dff8c https://hg.mozilla.org/integration/mozilla-inbound/rev/02784824ebf1

Something from this push broke weightmapping-12579.html on OSX 10.10: https://treeherder.mozilla.org/logviewer.html#?job_id=19315024&repo=mozilla-inbound Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/3ea7274d656d

For reference for myself. Here is a screen shot of reftest-analyzer. Right edges on each '600' - '900' were different. I am still investigating why the difference happens with patches for this bug and why the failure happens only on MacOS 10.10.

Hello, John san. This patch is the patch what I talked with you this afternoon. It's OK to use fuzzy-if here? What I've been observed about this failure reftest: * This failure can be reproduced only on MacOS 10.10 on try. * This failure can not be reproduced on local MacOS 10.10. (as far as I tried.) * Removing all '東京特許許可局' in the table does not cause the intermittent failure on try. * Removing all words whose font is thinner than 500 does not cause the intermittent failure on try. * Using only one downloadable(mplus-1p-medium) font does not cause the intermittent failure on try. * Replacing mplus-1p-thin and mplus-1p-light with mplus-1p-medium (there are 3 downloadable fonts in the document) does not cause the intermittent failure. I am going to file a new bug for this reftest issue once this patch gets reviewed. Thank you,

Comment on attachment 8709827 [details] [diff] [review] Use fuzzy-if for the failure reftest This is probably fine but I'd like to test this under 10.11 to see if I can reproduce it there. I'll do this tomorrow.

Comment on attachment 8709827 [details] [diff] [review] Use fuzzy-if for the failure reftest Looks fine to me. I tested under OSX 10.11 and the reftest passes. But I do think there's an underlying bug here associated with the load order of fonts. My guess is there an invalidation bug here such that depending upon the load order of fonts, the rendering will change.

Thank you, John-san! The previous patch does not have any spaces between fuzzy-if() and HTTP. This patch fixes it.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ea770e7b151 Now the reftest on Mac OS 10.10 seems good. https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0bd34e98513

https://hg.mozilla.org/integration/mozilla-inbound/rev/470e3a31a076 https://hg.mozilla.org/integration/mozilla-inbound/rev/9319f4591fc7 https://hg.mozilla.org/integration/mozilla-inbound/rev/1c0965533aef

https://hg.mozilla.org/mozilla-central/rev/470e3a31a076 https://hg.mozilla.org/mozilla-central/rev/9319f4591fc7 https://hg.mozilla.org/mozilla-central/rev/1c0965533aef