1231042 - Lockscreen briefly displays last displayed time when screen turned on before updating to current time.

Status: VERIFIED FIXED

(Core :: Graphics, defect, P1)

Description

[Martin Shuman [:Marty]]

Attachment: logcat_lockscreen-clock.txt

Description:
When the user presses the power button to turn the screen on, the lockscreen will briefly display the last displayed time before updating to the current time.

Repro Steps:
1) Update a Aries to 20151207143802
2) At the homescreen, make note of the time.
3) Lock the device.
4) Wait at least 2 minutes
5) Press the power button to unlock the device.

Actual:
Lockscreen briefly displays incorrect time before updating to the current time.

Expected:
Lockscreen only displays the current time.

Environmental Variables:
Device: Aries 2.6
Build ID: 20151207143802
Gaia: 24ed003a53a81f63367e265fa7117cbe7d23d4c8
Gecko: 59bc3c7a83de7ffb611203912a7da6ad84535a5a
Gonk: a19052e4389c3ae2d8fc3e7a74a475401baacc56
Version: 45.0a1 (2.6)
Firmware Version: D5803_23.1.A.1.28_NCB.ftf
User Agent: Mozilla/5.0 (Mobile; rv:45.0) Gecko/45.0 Firefox/45.0

Repro frequency: 10/10
See attached: Video (URL), Logcat

Comment 1

[Martin Shuman [:Marty]]

This issue DOES occur on the latest Flame 2.6 build.
Lockscreen briefly displays incorrect time before updating to the current time.

Environmental Variables:
Device: FlameKK 2.6 (512MB)
BuildID: 20151207030216
Gaia: 24ed003a53a81f63367e265fa7117cbe7d23d4c8
Gecko: 528ea05671e9bd9ccb33d1558a20691a72c85f98
Gonk: 205ac4204bbbb2098a8046444acba551ba5dc75a
Version: 45.0a1 (2.6) 
Firmware Version: v18D
User Agent: Mozilla/5.0 (Mobile; rv:45.0) Gecko/45.0 Firefox/45.0

********************************************

This issue does NOT occur on the latest Flame 2.5 build.
Lockscreen only displays the current time.

Environmental Variables:
Device: FlameKK 2.5 (512MB)
BuildID: 20151207121611
Gaia: 2d54c29f429bed790b5d8284633812dc2b782518
Gecko: c491dedc389de5c4686543b990c92d4f47715ee8
Gonk: 205ac4204bbbb2098a8046444acba551ba5dc75a
Version: 44.0a2 (2.5) 
Firmware Version: v18D
User Agent: Mozilla/5.0 (Mobile; rv:44.0) Gecko/44.0 Firefox/44.0

Comment 2

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

Let's get a window here.  Iris I don't think this issue is a blocker, but I'd like a second opinion.

Comment 3

[Pi Wei Cheng [:piwei] (inactive)]

b2g-inbound regression window:

Last Working
Device: Flame
BuildID: 20151113013830
Gaia: e6e6b3ab3bcf04431300d5c472f89771380baa02
Gecko: da1bcceb5f570ac5ece917c0daf9dd43453276f2
Version: 45.0a1 (2.6 Master) 
Firmware Version: v18Dv4
User Agent: Mozilla/5.0 (Mobile; rv:45.0) Gecko/45.0 Firefox/45.0

First Broken
Device: Flame
BuildID: 20151113020803
Gaia: 3fb68dc7084a1339eb18a87a1a593a51448e56a8
Gecko: e65a537f8ac3ae3320554d09bc0fd3589fef3d1c
Version: 45.0a1 (2.6 Master) 
Firmware Version: v18Dv4
User Agent: Mozilla/5.0 (Mobile; rv:45.0) Gecko/45.0 Firefox/45.0

Last Working Gaia First Broken Gecko - no repro
Gaia: e6e6b3ab3bcf04431300d5c472f89771380baa02
Gecko: e65a537f8ac3ae3320554d09bc0fd3589fef3d1c

Last Working Gecko First Broken Gaia - repro
Gaia: 3fb68dc7084a1339eb18a87a1a593a51448e56a8
Gecko: da1bcceb5f570ac5ece917c0daf9dd43453276f2

Gaia pushlog:
https://github.com/mozilla-b2g/gaia/compare/e6e6b3ab3bcf04431300d5c472f89771380baa02...3fb68dc7084a1339eb18a87a1a593a51448e56a8

This issue is likely caused by changes made in Bug 1188232

Comment 4

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

Ting-Yu this issue seems to have been caused by the changes for bug 1188232.  Can you please take a look?

Comment 5

[Ting-Yu Chou [:ting] (away)]

Yes, sure. Actually it's a known issue, I should have filed this earlier.

It's because the presshell is not yet visible when screen is on, it changes to visible asynchronously by sizemodechange event [1] which is from ScreenOnOffEvent [2].

[1] https://dxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#556
[2] https://dxr.mozilla.org/mozilla-central/source/widget/gonk/nsScreenManagerGonk.cpp#80

Comment 6

[Ting-Yu Chou [:ting] (away)]

This is a bit different from what I thought. The last displayed time can still be seen even the PresShell is active when WillPaint() is called. There's a short delay between screen on and updating the clock.

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

I'm trying to read the code in lockscreen/widget and the only thing that comes to mind is in how we handle ticks.



It seems that when screenchange event is fired we do not react if screen gets enabled at all. We only [1] transfer to tick but then we wait for lockscreen-notification-clock-tick to actually updateClock.

If my hypothesis is correct, all we'd need to do is updateClock in screenchange (and possibly updateFormatters if we're not listening to things like timeformatchange when suspended).


[0] https://github.com/mozilla-b2g/gaia/blob/5c79eeb800161651b22af9461e3de07e527a0141/apps/system/lockscreen/js/widgets/clock/lockscreen_clock_widget_tick.js#L56-L63

[1] https://github.com/mozilla-b2g/gaia/blob/5c79eeb800161651b22af9461e3de07e527a0141/apps/system/lockscreen/js/widgets/clock/lockscreen_clock_widget_suspend.js#L36

Comment 8

[Ting-Yu Chou [:ting] (away)]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #7)
> If my hypothesis is correct, all we'd need to do is updateClock in
> screenchange (and possibly updateFormatters if we're not listening to things
> like timeformatchange when suspended).

Thanks Zibi, but we also need some other changes to make sure the updated clock is painted before screen turns on.

Comment 11

[Johan Lorenzo [:jlorenzo]]

[Blocking Requested - why for this release]: Regression from 2.5, that can be seen by nearly every user. This regression can introduce the feeling that the product is not very fast enough.

Comment 12

[Ting-Yu Chou [:ting] (away)]

The light up sequence now is:

  1. navigator.mozPower.screenEnabled = true,
    a. nsScreenManagerGonk::DisplayEnabled()
    b. VsyncControl(true)
    c. DispathToMainThread(ScreenOnOffEvent) which triggers 3 and 4
  2. screenchange
    a. Transfer LockScreenClockWidgetSuspend to LockScreenClockWidgetTick
  3. screen-state-changed
  4. sizemodechange
    a. shell.js calls setVisible(true) for the browser iframe of system app
    b. sends async message browser-element-api:call with set-visible
    c. BrowserElementChild receives the message and sets docShell.isActive true
    d. sends async message visibilitychange to BrowserElementParent
  5. visibilitychange

Before bug 1188232, #screen will be set to black background and everything within it will be hidden by toggling a css class when screen is off, so when it turns on, you see nothing but black. But the restyle from removing the css class takes time.

I will try reorder, and still have screen on/off faster.

Comment 13

[Ting-Yu Chou [:ting] (away)]

Reorder the sequence doesn't seem going to work, even clock is updated before setting docShell.isActive true, CompositeToTarget() can still be called earlier than WillPaint(), will check how does vsync affect CompositorVsyncScheduler and RefreshDriverVsyncObserver.

Comment 14

[Ting-Yu Chou [:ting] (away)]

When vsync is reenabled from turnning screen on, NotifyVsync() of CompositorVsyncScheduler and RefreshDriverVsyncObserver will be called. But Compositor thread is free while main thread is usually busy, which if |mNeedsComposite > 0|, CompositeToTarget::CompositeToTarget() will be done prior nsRefreshDriver::Tick() and WillUpdate(), so the old screen content shows up.

Also it seems Compositor code does not care about screen on/off state, which is why |mNeedsComposite| can be set to >0 when screen is off. I am not sure what's the design here, it may be intended.

Jerry, do you have any ideas about this? Thanks.

Comment 15

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Maybe the problem is that the touch events are sent during the screen off at [1], and then mNeedsComposite increased. When the first vsync tick comes after screen on, the compositor compose the layer data which is the previous frame's data.

When does the first nsRefreshDriver::Tick() tick? I think we only see the old frame in a brief time just before the first nsRefreshDriver::Tick(). The new content will be sent to compositor in nsRefreshDriver::Tick().  

[1]
https://hg.mozilla.org/mozilla-central/annotate/388bdc46ba51ee31da8b8abe977e0ca38d117434/widget/gonk/GeckoTouchDispatcher.cpp#l108

Comment 16

[Ting-Yu Chou [:ting] (away)]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #15)
> When does the first nsRefreshDriver::Tick() tick? I think we only see the
> old frame in a brief time just before the first nsRefreshDriver::Tick(). The
> new content will be sent to compositor in nsRefreshDriver::Tick().  

Yes, the old frame shows up briefly as the bug described. But we prefer not to have it.

Comment 17

[Gregor Wagner [:gwagner]]

Visible foxfood regression on prominent UI.

Comment 18

[Ting-Yu Chou [:ting] (away)]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #15)
> Maybe the problem is that the touch events are sent during the screen off at
> [1], and then mNeedsComposite increased. When the first vsync tick comes
> after screen on, the compositor compose the layer data which is the previous
> frame's data.

There're many places could trigger ScheduleComposition() and SetNeedsComposite(), and from logs it is usually CompositorParent::ShadowLayersUpdated() from LayerTransactionParent::RecvUpdate().

I guess we should somehow invalidate mNeedsComposite if it is going to be or is >0 after screen/vsync off, and leave the first frame to application.

Comment 19

[Jerry Shih[:jerry] (UTC+8) (inactive)]

If we want to show the correct result, the screen-on process should wait for the first refresh driver tick(). The layer data at compositor side is still the old time info until next refresh driver tick. Since we need to wait the content to update the correct time data, the screen-on process will become a little bit slower.

Comment 20

[Ting-Yu Chou [:ting] (away)]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #19)
> If we want to show the correct result, the screen-on process should wait for
> the first refresh driver tick(). The layer data at compositor side is still
> the old time info until next refresh driver tick. Since we need to wait the
> content to update the correct time data, the screen-on process will become a
> little bit slower.

Wouldn't there still be chances that screen goes on before compositor thread composites to target? How about the idea of invalidating mNeedsComposite, as it guarantees the first frame will be from the first round of refresh driver tick.

Comment 21

[Jerry Shih[:jerry] (UTC+8) (inactive)]

It's more reasonable that make "mNeedsComposite" recount when screen on. Let me think how to do that.

Comment 22

[Ting-Yu Chou [:ting] (away)]

Just had a offline discussion with Jerry.

We both agree that invalidating mNeedsComposite when screen gets off is reasonable. But he also reminded me that there could be devices (not Aries) showing the last screen directly whenever it turns on, even CompositeToTarget() is not called.

He will work out a patch for mNeedsComposite, and meanwhile I'll check how does Android handle this.

Comment 23

[Ting-Yu Chou [:ting] (away)]

On Android, SurfaceFlinger will do repaintEverything() after calling unblank(), and the behavior of unblank() is implemented in fb_blank() of video driver. I couldn't find any document of fb_blank(), not sure what is expected after unblanking.

Comment 24

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: clear |SetNeedsCompositeTask| status in CompositorParent when screen on. v1

Comment 25

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: clear |SetNeedsCompositeTask| status in CompositorParent when screen on. v2

fix CancelCurrentSetNeedsCompositeTask() thread model.

Comment 26

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8701359 [details] [diff] [review]
clear |SetNeedsCompositeTask| status in CompositorParent when screen on. v2

Review of attachment 8701359 [details] [diff] [review]:
-----------------------------------------------------------------

When screen off, some vsync observer still request vsync(e.g. refresh driver). Then the mNeedsComposite becomes bigger than 0. When screen on, vsync event starting, compositor will compose the old layer tree due to mNeedsComposite>0. Thus, we will see the old time at lockscreen.

This patch try to reset mNeedsComposite at screen-on.

::: gfx/layers/ipc/CompositorParent.cpp
@@ +360,5 @@
>    }
>  }
>  
>  void
>  CompositorVsyncScheduler::CancelCurrentSetNeedsCompositeTask()

This will be called from nsScreenManagerGonk::DisplayEnabled at main thread.
Post to compositor thread.

::: widget/gonk/nsScreenManagerGonk.cpp
@@ +862,5 @@
>      return NS_OK;
>  }
> +
> +void
> +nsScreenManagerGonk::SetCompositorVsyncScheduler(mozilla::layers::CompositorVsyncScheduler *aObserver)

As https://hg.mozilla.org/mozilla-central/annotate/388bdc46ba51ee31da8b8abe977e0ca38d117434/widget/gonk/GeckoTouchDispatcher.cpp#l90

Comment 27

[Jerry Shih[:jerry] (UTC+8) (inactive)]

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

Comment 28

[Benoit Girard (:BenWa)]

Comment on attachment 8701359 [details] [diff] [review]
clear |SetNeedsCompositeTask| status in CompositorParent when screen on. v2

Review of attachment 8701359 [details] [diff] [review]:
-----------------------------------------------------------------

I think it's worth having mchang look at it too.

::: widget/gonk/nsScreenManagerGonk.cpp
@@ +624,5 @@
> +    if (mCompositorVsyncScheduler) {
> +        if (aEnabled) {
> +            // Clear all pending "NeedsComposite" flag since we are ready for
> +            // new vsync period.
> +            mCompositorVsyncScheduler->CancelCurrentSetNeedsCompositeTask();

I could maybe understand canceling pending composite if we're disabling the display. But I don't understand why we would cancel composite task as we're re-enabling the display.

If we have stale composite task they should of been thrown out when we we're stopping.

Comment 29

[Benoit Girard (:BenWa)]

I read through the bug and it make sense. However I'd rather that we force 'mNeedsComposite' to stay at zero when the screen is off then arbitrary cancel it when the screen is off and/or the compositor is paused IMO. I'm affraid that we would cancel a request for putting out the very first frame as we're re-enabling the screen/compositor/refresh driver.

Or maybe you can convince me that this is ok?

Comment 30

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Benoit Girard (:BenWa) from comment #29)
> I read through the bug and it make sense. However I'd rather that we force
> 'mNeedsComposite' to stay at zero when the screen is off then arbitrary
> cancel it when the screen is off and/or the compositor is paused IMO. I'm
> affraid that we would cancel a request for putting out the very first frame
> as we're re-enabling the screen/compositor/refresh driver.
> 
> Or maybe you can convince me that this is ok?


 void
 nsScreenManagerGonk::DisplayEnabled(bool aEnabled)
 {
+    if (mCompositorVsyncScheduler) {
+        if (aEnabled) {
+            mCompositorVsyncScheduler->CancelCurrentSetNeedsCompositeTask();
+        }
+    }
+
     VsyncControl(aEnabled);
     NS_DispatchToMainThread(aEnabled ? mScreenOnEvent : mScreenOffEvent);
 }


If the composition request comes "before DisplayEnabled(true)", I think we should skip this request. Otherwise, we might still see the old(or unwanted) result.

Comment 31

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8701359 [details] [diff] [review]
clear |SetNeedsCompositeTask| status in CompositorParent when screen on. v2

Review of attachment 8701359 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/gonk/nsScreenManagerGonk.cpp
@@ +624,5 @@
> +    if (mCompositorVsyncScheduler) {
> +        if (aEnabled) {
> +            // Clear all pending "NeedsComposite" flag since we are ready for
> +            // new vsync period.
> +            mCompositorVsyncScheduler->CancelCurrentSetNeedsCompositeTask();

I agree with this. Can we clear out all composite tasks when we disable the display instead? Then when the display is turned on, we can go through the normal rendering pipeline and we should only composite after the refresh driver has ticked and we got a layer transaction.

Comment 32

[Benoit Girard (:BenWa)]

I don't think the current approach is correct. If we get composition requests while the display is enabled then we should ignore it. We shouldn't remove them after we re-enable it. We should avoid having them there in the first place.

Comment 33

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: clear |SetNeedsCompositeTask| status in CompositorParent when screen on. v3

Comment 34

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8704009 [details] [diff] [review]
clear |SetNeedsCompositeTask| status in CompositorParent when screen on. v3

Review of attachment 8704009 [details] [diff] [review]:
-----------------------------------------------------------------

This patch export a |SetDisplay()| function for screen on/off. When the screen off, it will cancel all pending composition task and related flag in vsync scheduler.

::: gfx/layers/ipc/CompositorParent.cpp
@@ +343,5 @@
> +    return;
> +  }
> +
> +  mDisplayEnable = aDisplayEnable;
> +  if (!mDisplayEnable) {

When screen off, reset all composition task and flag.

@@ +350,5 @@
> +  }
> +}
> +
> +void
> +CompositorVsyncScheduler::CancelSetDisplayTask()

We have 3 similar pairs of set/cancelTask function. Maybe there is another approach to do these.

::: gfx/layers/ipc/CompositorParent.h
@@ +175,5 @@
>    mozilla::Monitor mSetNeedsCompositeMonitor;
>    CancelableTask* mSetNeedsCompositeTask;
> +
> +  mozilla::Monitor mSetDisplayMonitor;
> +  CancelableTask* mSetDisplayTask;

Create similar monitor and task as mCurrentCompositeTask and mSetNeedsCompositeTask do.

That prevent the data racing and unfinished message loop task problem in ~CompositorVsyncScheduler()

Comment 35

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8704009 [details] [diff] [review]
clear |SetNeedsCompositeTask| status in CompositorParent when screen on. v3

Review of attachment 8704009 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think we should take of the screen enabled case. That should already be enabled when we turn on the screen and schedule a paint through the normal rendering pipeline. I think we only have to take care of the screen disabled case. When the screen is off, clear out the composite tasks.

::: gfx/layers/ipc/CompositorParent.cpp
@@ +445,5 @@
>      MonitorAutoLock lock(mSetNeedsCompositeMonitor);
>      mSetNeedsCompositeTask = nullptr;
>    }
>  
> +  if (!mDisplayEnable) {

Does this actually happen? Are we trying to paint things when the screen is off? HwcComposer should be disabling vsync, so we shouldn't actually be painting / compositing anything.

Comment 36

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8704009 [details] [diff] [review]
clear |SetNeedsCompositeTask| status in CompositorParent when screen on. v3

Review of attachment 8704009 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/CompositorParent.cpp
@@ +445,5 @@
>      MonitorAutoLock lock(mSetNeedsCompositeMonitor);
>      mSetNeedsCompositeTask = nullptr;
>    }
>  
> +  if (!mDisplayEnable) {

Yes, there are still some SetNeedsComposite() requests when screen is off. The number is 2 or 3 at the beginning of screen-off. I think that might come from GonkTouch or refreshDriver.

Skip the request here is simpler. Or we need to check the refreshDriver and GonkTouch behavior when screen-off.

Comment 37

[Jerry Shih[:jerry] (UTC+8) (inactive)]

comment 36

Comment 38

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Here is the call stack that we still call SetNeedsComposite() when screen is off.

We turn off the screen. Meanwhile, content sends a layer update. If the layer update message is processed after screen off, we will hit the situation in comment 35.

#0  mozilla::layers::CompositorVsyncScheduler::SetNeedsComposite
#1  0xb398c2ea in mozilla::layers::CompositorVsyncScheduler::ScheduleComposition
#2  0xb398c2f8 in mozilla::layers::CompositorParent::ScheduleComposition
#3  0xb3993e90 in mozilla::layers::CompositorParent::ShadowLayersUpdated
#4  0xb39959f0 in mozilla::layers::LayerTransactionParent::RecvUpdate

Comment 39

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8704009 [details] [diff] [review]
clear |SetNeedsCompositeTask| status in CompositorParent when screen on. v3

Review of attachment 8704009 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/CompositorParent.cpp
@@ +286,5 @@
>  
>  CompositorVsyncScheduler::CompositorVsyncScheduler(CompositorParent* aCompositorParent, nsIWidget* aWidget)
>    : mCompositorParent(aCompositorParent)
>    , mLastCompose(TimeStamp::Now())
> +  , mDisplayEnable(false)

nit: rename mDisplayEnable to mDisplayEnabled.

@@ +303,5 @@
>    mVsyncObserver = new Observer(this);
>    mCompositorVsyncDispatcher = aWidget->GetCompositorVsyncDispatcher();
>  #ifdef MOZ_WIDGET_GONK
>    GeckoTouchDispatcher::GetInstance()->SetCompositorVsyncScheduler(this);
> +

nit: Instead of adding a ref ptr, just do nsScreenMAnagerGonk::GetInstance()->SetCompositorVsyncScheduler(this);

@@ +323,5 @@
>    mCompositorParent = nullptr;
>    mCompositorVsyncDispatcher = nullptr;
>  }
>  
>  void

nit: please add a comment as to why we need these functions. Also, please wrap around via #ifdef MOZ_WIDGET_GONK.

@@ +326,5 @@
>  
>  void
> +CompositorVsyncScheduler::SetDisplay(bool aDisplayEnable)
> +{
> +  if (!CompositorParent::IsInCompositorThread()) {

What thread is this normally called on? Please add a comment here.

Comment 40

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8704009 [details] [diff] [review]
clear |SetNeedsCompositeTask| status in CompositorParent when screen on. v3

Review of attachment 8704009 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/CompositorParent.cpp
@@ +286,5 @@
>  
>  CompositorVsyncScheduler::CompositorVsyncScheduler(CompositorParent* aCompositorParent, nsIWidget* aWidget)
>    : mCompositorParent(aCompositorParent)
>    , mLastCompose(TimeStamp::Now())
> +  , mDisplayEnable(false)

checked.

@@ +303,5 @@
>    mVsyncObserver = new Observer(this);
>    mCompositorVsyncDispatcher = aWidget->GetCompositorVsyncDispatcher();
>  #ifdef MOZ_WIDGET_GONK
>    GeckoTouchDispatcher::GetInstance()->SetCompositorVsyncScheduler(this);
> +

nsScreenMAnagerGonk::GetInstance() returns |already_AddRefed<nsScreenManagerGonk>|, so we can't just do |nsScreenMAnagerGonk::GetInstance()->SetCompositorVsyncScheduler(this)|

@@ +323,5 @@
>    mCompositorParent = nullptr;
>    mCompositorVsyncDispatcher = nullptr;
>  }
>  
>  void

checked

@@ +326,5 @@
>  
>  void
> +CompositorVsyncScheduler::SetDisplay(bool aDisplayEnable)
> +{
> +  if (!CompositorParent::IsInCompositorThread()) {

checked

Comment 41

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: clean current composition task and related flag when screen off. v4

update for comment 39.

Comment 42

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: clean current composition task and related flag when screen off. v5

update for comment 39.
add missed ifdef.

Comment 43

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8706221 [details] [diff] [review]
clean current composition task and related flag when screen off. v5

Review of attachment 8706221 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/CompositorParent.cpp
@@ +308,5 @@
>    mCompositorVsyncDispatcher = aWidget->GetCompositorVsyncDispatcher();
>  #ifdef MOZ_WIDGET_GONK
>    GeckoTouchDispatcher::GetInstance()->SetCompositorVsyncScheduler(this);
> +
> +  RefPtr<nsScreenManagerGonk> screenManager = nsScreenManagerGonk::GetInstance();

nit: Please make this like line 310. nsScreenManagerGonk::GetInstance()->SetCompositorVsyncScheduler(this);

@@ +344,5 @@
> +  } else {
> +    MonitorAutoLock lock(mSetDisplayMonitor);
> +    mSetDisplayTask = nullptr;
> +  }
> +

nit: Assert NS_IsMainThread() then.

::: gfx/layers/ipc/CompositorParent.h
@@ +170,5 @@
>    TimeStamp mExpectedComposeStartTime;
>  #endif
>  
> +#ifdef MOZ_WIDGET_GONK
> +  bool mDisplayEnabled;

nit: move this all under 1 ifdef MOZ_WIDGET_GONK with the other added fields.

Comment 44

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8706221 [details] [diff] [review]
clean current composition task and related flag when screen off. v5

r+ with the nits fixed. Thanks!

Comment 45

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P1: clean current composition task and related flag when screen off. v6. r=mchang

update for comment 43

Comment 46

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8707317 [details] [diff] [review]
P1: clean current composition task and related flag when screen off. v6. r=mchang

Review of attachment 8707317 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/CompositorParent.cpp
@@ +307,5 @@
>  #ifdef MOZ_WIDGET_GONK
>    GeckoTouchDispatcher::GetInstance()->SetCompositorVsyncScheduler(this);
> +
> +  RefPtr<nsScreenManagerGonk> screenManager = nsScreenManagerGonk::GetInstance();
> +  screenManager->SetCompositorVsyncScheduler(this);

We can't do that as |GeckoTouchDispatcher::GetInstance()->SetCompositorVsyncScheduler(this)|, since already_AddRefed doesn't have |operator->()|.
So, I make a RefPtr here.

Comment 47

[Jerry Shih[:jerry] (UTC+8) (inactive)]

try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=49f248d837d0

Comment 48

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/15145a32cd19

Comment 49

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out because it looks like this caused various b2g reftest failures: https://hg.mozilla.org/integration/mozilla-inbound/rev/f1ebaf0ca071

https://treeherder.mozilla.org/logviewer.html#?job_id=19744758&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=19749419&repo=mozilla-inbound

Comment 50

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/15145a32cd19

Comment 51

[Ryan VanderMeulen [:RyanVM]]

Sorry, that shouldn't have gotten merged around. Backout grafted and merged around.
https://hg.mozilla.org/mozilla-central/rev/4e2224c009df

Comment 52

[Jerry Shih[:jerry] (UTC+8) (inactive)]

a) table-row-opacity-dynamic-1.html
https://treeherder.mozilla.org/logviewer.html#?job_id=19749419&repo=mozilla-inbound

For this failed, I can 100% reproduce the same failed locally "without" my match. I just test with the clear master branch. So I'm not sure the failed reason. Do we have the mozilla-inbound try link which contain this bug's patch?

result:
REFTEST TEST-UNEXPECTED-FAIL | http://10.247.24.93:8888/tests/layout/reftests/table-background/table-row-opacity-dynamic-1.html | image comparison (==), max difference: 1, number of differing pixels: 25

my gecko version(the version which back out the bug 1231042):
commit b69d9b88c50001fd0e7bc4b2e1afe0145e77a3a0
Author: Wes Kocher <[e-mail]>
Date:   Wed Jan 13 12:57:45 2016 -0800
    Backed out changeset 15145a32cd19 (bug 1231042) for b2g reftest bustage. a=backout
    --HG--
    extra : source : f1ebaf0ca07195b30c8f6deec5e0131043fa17c2

b) scroll-behavior-6.html
I can't reproduce the failed locally. Still trying.

Comment 53

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Here's all of the test runs on inbound from the push prior to yours up to the backout to show when they started failing and got fixed by the backout:

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-searchStr=b2g%20reftest%20%28r15&fromchange=2038722d99d4&tochange=f1ebaf0ca071&group_state=expanded

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-searchStr=b2g%20reftest%20%28r18&fromchange=2038722d99d4&tochange=f1ebaf0ca071&group_state=expanded

Comment 54

[Jerry Shih[:jerry] (UTC+8) (inactive)]

I think I find the failed reason. In reftest, I don't see the display on/off event. So the composition request is always skipped. That might break the tests.

Wait for the results for these two approaches:
a)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c91121e3158a
b)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d3e52d2f8c7

Comment 56

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P2: Init |mDisplayEnable| with true.

Here are R15 and R18 reftest failed with P1 patch.
reftest framework doesn't set the display on, so all composition request will just skip.
This patch try to use "true" as the default value. It's a little bit strange, but the original system behavior is like we assume the display on(we never skip the compostion request).

Comment 57

[Jerry Shih[:jerry] (UTC+8) (inactive)]

try for P1 and P2 patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbdc01ae2a07

Comment 58

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8708213 [details] [diff] [review]
P2: Init |mDisplayEnable| with true.

Review of attachment 8708213 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/CompositorParent.cpp
@@ +296,5 @@
>    , mSetNeedsCompositeTask(nullptr)
>  #ifdef MOZ_WIDGET_GONK
> +  // Init this value as TRUE, since some emulators never trigger the display on/off.
> +  // And the reftest framework doesn't turn on the display either.
> +  , mDisplayEnabled(true)

Please wrap this around an ifdef emulator / detect if we're in a test environment only. Is that possible?

Comment 59

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Mason Chang [:mchang] from comment #58)
> Comment on attachment 8708213 [details] [diff] [review]
> P2: Init |mDisplayEnable| with true.
> 
> Review of attachment 8708213 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ipc/CompositorParent.cpp
> @@ +296,5 @@
> >    , mSetNeedsCompositeTask(nullptr)
> >  #ifdef MOZ_WIDGET_GONK
> > +  // Init this value as TRUE, since some emulators never trigger the display on/off.
> > +  // And the reftest framework doesn't turn on the display either.
> > +  , mDisplayEnabled(true)
> 
> Please wrap this around an ifdef emulator / detect if we're in a test
> environment only. Is that possible?

I'm trying to find that. So far, I don't know a good def for this case.

Comment 60

[Jerry Shih[:jerry] (UTC+8) (inactive)]

The emulator-kk will send the screen on call during normal booting and reftest, but emulator-ics won't.
The ics is phased out. I think we don't need to spend time for ics.
May I just make all MOZ_WIDGET_GONK def with android KK version checking for part 1 patch and skip the part 2?

Comment 61

[Mason Chang [Inactive] [:mchang]]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #60)
> The emulator-kk will send the screen on call during normal booting and
> reftest, but emulator-ics won't.
> The ics is phased out. I think we don't need to spend time for ics.
> May I just make all MOZ_WIDGET_GONK def with android KK version checking for
> part 1 patch and skip the part 2?

Sure, thanks for checking.

Comment 62

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: clean current composition task and related flag when screen off. v7. r=mchang

only apply this patch for android KK and upon

Comment 63

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Please land the attachment 8709815 [details] [diff] [review] to m-c.

Try link:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ab619abc525

Comment 64

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7333e869b284

Comment 65

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7333e869b284

Comment 66

[Adam Alldredge [:AdamA][Inactive]]

Environmental Variables:
Device: Aries 2.6 [Full Flash]
BuildID: 20160224110426
Gaia: 4f0e2a1a42a2d049b6fe8f4f095cdcdf0fd5465c
Gecko: d848a5628d801a460a7244cbcdea22d328d8b310
Gonk: a19052e4389c3ae2d8fc3e7a74a475401baacc56
Version: 47.0a1 (2.6) 
Firmware Version: D5803_23.1.A.1.28_NCB.ftf
User Agent: Mozilla/5.0 (Mobile; rv:47.0) Gecko/47.0 Firefox/47.0

Device: FlameKK 2.6 [Full Flash][512mb]
BuildID: 20160225030411
Gaia: 4f0e2a1a42a2d049b6fe8f4f095cdcdf0fd5465c
Gecko: c1e0d1890cfee9d86c8d566b0490053f21e0afc6
Gonk: 8a066f7fa7410e32b58def35f322aa33f03db283
Version: 47.0a1 (2.6) 
Firmware Version: v18D v5
User Agent: Mozilla/5.0 (Mobile; rv:47.0) Gecko/47.0 Firefox/47.0

Result:
The phone shows the correct time when unlocking the phone.