+++ This is a followup of Bug #877215 +++ 1. Title : Device does not enter cpu idle state (AP's core 0 voltage) 2. Precondition : Turn on deivce 3. Tester's Action : Check device state after turn on. 4. Detailed Symptom : When device is turned on, the AP's core voltage never goes down. 5. Expected : Device should enter cpu idle state (AP's core 0 voltage), when there is no input for certain period of time. 6. Reproducibility: Y 1) Frequency Rate : 100% 7. Gaia revision : As per the comment 12 of Bug #877215 filing a follow up bug.

ni? alive/yuren as per https://bugzilla.mozilla.org/show_bug.cgi?id=877215#c14 would be great if you guys can look at this. thanks!

Revert this patch still got some process hold the "unknown_suspend" wakelock "Bug 877214 - Toggle lockscreen animation according to visibilitychange"

Revert? What do you mean?

Rollback the Gaia change, but Leo say that wakelock.is normal. https://github.com/mozilla-b2g/gaia/commit/0bfcaf8546c6d365b0474d980a154d859d22c872

Why did you roll back the change? Is it wrong?

Hi Andreas, It just for my local test, not blackout it from repository.

Why did you back it out locally?

Hi Andreas, I'm curious about Leo say this patch cause this problem: https://bugzilla.mozilla.org/show_bug.cgi?id=877215#c13 Leo 2013-05-31 03:10:29 PDT When I role back patch of Bug 838486(apply master git on 3/13), tester said device can enter CPU Idle state. ---- I will try to compare the idle current between Leo and Unagi.

Hi all, I think wakelock is only relevant to "device suspend state" which is made by pressing power key or timeout of idle timer from system app. But the bug here mentioned to "CPU Idle State" which is no relationship with wakelock. It is depend on the timeout value of nearest timer. When kernel's idle thread (lowest priority) got the chance to run it will detect the value of next timer then decide the power saving level (spin, SWFI, Standalone power collapse). In this case, it may caused by some Gaia/Gecko components fired a shorter timer periodically. Hi Leo, Could you check the reason for cpu can't go into idle state in kernel's point of view? If shorter time is the reason then we can help to check on Gaia/Gecko side.

I'll help Randy to investigate this issue.

Thanks Marco! Thats what I suspected. Yuren, you guys should run the gecko process in GDB and break on the main event loop and see how often we wake up while sleeping and then figure out why.

Update to 0602 mozilla-b2g18-leo-eng build, turn the back light to minimal, the current is 45mA. After stop b2g the current is down to ~43ma. check the top -m 10 -t -d 10 865 865 0 1% S 199756K 61620K fg root b2g /system/b2g/b2g 161 212 0 1% S 5908K 468K fg root sensord /system/bin/sensord 865 882 0 1% S 199756K 61620K fg root Timer /system/b2g/b2g 1097 1097 0 0% R 1072K 464K fg root top top

Profile the b2g process, found nsRefreshDriver::Notify.

Can you post a stack?

also found CC::nsCycleCollector_collect, GC::GarbageCollectNow hit many times on Timer.

logging at nsTimerImpl.cpp:475, mClosure, callback.c, callbackType, time 0x0 cb = 0x48101de0 type = 1 , time:19:58:07 294705 0x0 cb = 0x48101de0 type = 1 , time:19:58:07 311886 0x0 cb = 0x48101de0 type = 1 , time:19:58:07 329220 0x0 cb = 0x48101de0 type = 1 , time:19:58:07 346677 0x0 cb = 0x48101de0 type = 1 , time:19:58:07 363553 0x0 cb = 0x48101de0 type = 1 , time:19:58:07 380765 0x0 cb = 0x48101de0 type = 1 , time:19:58:07 397977 0x0 cb = 0x48101de0 type = 1 , time:19:58:07 415097 0x0 cb = 0x48101de0 type = 1 , time:19:58:07 432187 0x0 cb = 0x48101de0 type = 1 , time:19:58:07 449460 0x0 cb = 0x48101de0 type = 1 , time:19:58:07 466611 0x0 cb = 0x48101de0 type = 1 , time:19:58:07 483761 (gdb) p callback.c $1 = (nsTimerCallbackFunc) 0x48101de0 Is this normal?

keep nsRefreshDriver::Notify(nsITimer *aTimer)... notify on nsRefreshDriver 3858 0x0 cb = 0x450e80c0 type = 1 , time:23:59:50 899014 notify on nsRefreshDriver 3985 0x0 cb = 0x42becf50 type = 1 , time:23:59:50 902798 notify on nsRefreshDriver 3858 0x0 cb = 0x450e80c0 type = 1 , time:23:59:50 916562 notify on nsRefreshDriver 3985 0x0 cb = 0x42becf50 type = 1 , time:23:59:50 920590 notify on nsRefreshDriver 3858 0x0 cb = 0x450e80c0 type = 1 , time:23:59:50 934354 notify on nsRefreshDriver 3985 0x0 cb = 0x42becf50 type = 1 , time:23:59:50 938626 notify on nsRefreshDriver 3858 0x0 cb = 0x450e80c0 type = 1 , time:23:59:50 950925 But I found sometimes I can't found this symption by disable home screen directly, but keep refresh when enter setting apps.

Leo has provided information to identify the CPU has entered idle mode or not. We can cat /proc/msm_pm_stats and check idle-power-collapse item to see if count has increased or not. But this method isn't accurate. The better way is checking the clk of cpu and check the wave form.

It looks like the animation of lock-screen is not ended after unlocked. Just remove the animation works, but reset animation to none doesn't work. Strange.

Can apply this patch to check if the nsRefreshDriver timer is stoped or not. Directly run b2g.sh and check the console's output log timimg is keep changing. 7302 0x0 cb = 0x47e623d0 type = 1 , time:6:27:26 893244

Randy try to figure out why the refresh driver doesn't stop.

OK, I am digging out the root-casue. Found the timer is failed to be removed, a little strange.

made a webpage with the same animation as lockscreen. http://yurenju.github.io/animationtest/

In nsRefreshDriver.cpp, When this symption happen, the mObservers[0].Length() keeps > 0 (Flush_Style) , and ObserverCount() keeps return 1, so the StopTimer() won't be called, should found which one miss call the RemoveRefreshObserver().

(In reply to Randy Lin [:rlin] from comment #24) > In nsRefreshDriver.cpp, When this symption happen, the > mObservers[0].Length() keeps > 0 (Flush_Style) , and ObserverCount() keeps > return 1, so the StopTimer() won't be called, should found which one miss > call the RemoveRefreshObserver(). Did you manage to reproduce this behaviour on desktop or only on a phone? I am looking at desktop (with OMTC and OMTA to be a bit closer to b2g-land) and I do not observe this - I see StopTimer getting called from nsRefreshDriver::Tick, but in RemoveRefreshDriver we sometimes don't stop the timer because there are other refresh drivers still on the timer (in fact we seem to get 'locked into' this situation and never get down to zero refresh drivers, although not every run). On the phone does the refresh driver always keep going or only sometimes?

Hi Nick, We found this issue at b2g18 branch on b2g platform. You can check the refresh driver behavior after unlocking the cell phone. But if I disabled the lock screen through "setting->phone lock->disable" then re-boot the device, this symptom was gone.

Using an up to date Gecko, this does not seem to be a problem, or at least I could not repro. Trying with b2g18 now...

Hmm, so far I have not managed to observe this behaviour - the timer on the refresh driver is being stopped every now and again. So StopTimer does get called. The only odd thing I noticed is that RmoveRefreshObserver gets called twice for each observer, but this doesn't seem to harm anything.

After a few more runs I've noticed some pretty long periods of time where the refresh driver keeps going, but it always eventually stops. I kind of assume it is meant to be on - perhaps it is when I've left the lock screen going. Perhaps I'm not clear on the STR - I am unlocking the device, then using some things from the home screen and noting if the refresh driver is always on. Also tried unlocking and then doing nothing. I have also waited for the animation to play and then unlock. Are we instead talking about not unlocking and letting the screen turn off? Or not unlocking whilst the screen remains on?

Hi Nick, We found there are some code follow invoke the AddElementData() in nsAnimationManager, but miss the RemoveAllElementData(), is it normal? The STR is 1. boot-up device, 2. unlock lockscreen, enter home screen 3. keep the screen always on.

(In reply to Randy Lin [:rlin] from comment #30) > Hi Nick, > We found there are some code follow invoke the AddElementData() in > nsAnimationManager, but miss the RemoveAllElementData(), is it normal? I think it could be normal, the transition or animation managers could remove a single element data manually, as long as they call ElementDataRemoved afterwards then it is OK. If they don't call that, then we could get this kind of error. > The STR is 1. boot-up device, 2. unlock lockscreen, enter home screen 3. > keep the screen always on. Thanks! I think I covered doing this and couldn't repro :-( Exactly which b2g gecko repo are you using (I pulled b2g18 with no suffix).

The symptom is also seen on m-c. What is seen is that ElementAnimation instances are created but Destroy() is never called for them. nsAnimationManager only Destroy() an ElementAnimation instance in nsAnimationManager::CheckAnimationRule() but from the tests we never reach there.

jet can we get some help here? this is probably dbaron or bz territory

This patch causes the element data to actually get removed once the animation ends. I'm not 100% sure it's the right approach, but lets see if it works first. It works for me on desktop, but I couldn't repro the power issue. Could one of you see if this fixes the issue on a device please?

Hi Nick, By observing the invoke of the nsRefreshDriver::Tick function call, apply your patch and seems helpful on mozilla_central branch.

Comment on attachment 765711 [details] [diff] [review] possible fix by not adding empty animations in BuildAnimations Extra refresh driver tick is not seen after applying this patch.

Also test on b2g-18 branch, it works. No extra invoke of the nsRefreshDriver::Notify function.

Check the idle-power-collapse in /proc/msm_pm_stats, The count increases idle-power-collapse: count: 67 total_time: 2.523467881 =================================================== idle-power-collapse: count: 2267 total_time: 129.211636297

Comment on attachment 765711 [details] [diff] [review] possible fix by not adding empty animations in BuildAnimations This changes the behavior when there are dynamic changes to animation rules. We currently (per spec, I believe) preserve the start time and delay state of an animation as long as the animation is in the list of animations; this adds some complex conditions to that that I wouldn't want to expose to the Web. Instead, I think CommonElementAnimationData should have a virtual method for whether that AnimationData/TransitionData requires refreshes: transitions should return true, and animations should return its mNeedsRefreshes. Then CommonAnimationManager::AddElementData and CommonAnimationManager::ElementDataRemoved should call a common method that uses said method (plus a boolean to track whether the manager is currently a refresh observer), and nsAnimationManager should call that method as well when it might have changed mNeedsRefreshes. Or something like that. (Yeah, this was something I meant to do after implementing animations but never got back to doing. Bug 649238. Yikes.)

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #39) > Instead, I think CommonElementAnimationData should have a virtual method for > whether that AnimationData/TransitionData requires refreshes: transitions > should return true, and animations should return its mNeedsRefreshes. Then > CommonAnimationManager::AddElementData and > CommonAnimationManager::ElementDataRemoved should call a common method that > uses said method (plus a boolean to track whether the manager is currently a > refresh observer), and nsAnimationManager should call that method as well > when it might have changed mNeedsRefreshes. Or something like that. Or maybe, instead of virtual methods, nsTransitionManager should keep the current code (moving from CommonAnimationManager) and nsAnimationManager should just have separate code like the above.

dbaron: Is this what you mean? I think I followed your suggestion, but I wasn't 100% clear. We seem to call CheckNeedsRefresh() a lot, but I couldn't find a way to reduce the number of calls. Not sure if it will be a performance issue, hopefully it's not too bad. rlin: I see this reducing load on the refresh driver in the same was as the last patch on desktop, but if you could double check on a device, that would be great!

and a version which actually builds (thanks to rlin for finding the bug, and verifying the previous version worked).

Comment on attachment 765869 [details] [diff] [review] be more precise about removing the animation manager from the refresh driver ElementDataRemoved and AddElementData should be both MOZ_OVERRIDE *and* MOZ_FINAL on the transition manager and animation manager... at least assuming you can do that. I think the removal of the ElementDataRemoved call from FlushTransitions was a mistake -- that breaks exactly this in the transition manager. I don't see why you're adding a distinction between GetPositionInIteration and GetPositionInIterationInternal when you're not changing anything in either function except for the fact that GetPositionInIteration doesn't pass through the last 3 parameters to GetPositionInIterationInternal. You should undo that. I need to think a little more about whether the CheckNeedsRefresh calls are in sufficient places.

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #43) > Comment on attachment 765869 [details] [diff] [review] > be more precise about removing the animation manager from the refresh driver > > ElementDataRemoved and AddElementData should be both > MOZ_OVERRIDE *and* MOZ_FINAL on the transition manager and > animation manager... at least assuming you can do that. Er, I guess it's the clases nsAnimationManager and nsTransitionManager that should be marked as MOZ_FINAL. (The reason I care is that hopefully that will allow the compiler to get rid of the virtual function call overhead when the call is from within the class rather than from the base class.)

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #43) > Comment on attachment 765869 [details] [diff] [review] > be more precise about removing the animation manager from the refresh driver > > ElementDataRemoved and AddElementData should be both > MOZ_OVERRIDE *and* MOZ_FINAL on the transition manager and > animation manager... at least assuming you can do that. > OK done, on the class. > I think the removal of the ElementDataRemoved call from > FlushTransitions was a mistake -- that breaks exactly this > in the transition manager. > I think this is ok because ElementDataRemoved gets called from the destructor of CommonElementAnimationData. I think I observe this behaviour. Am I missing something? > > I don't see why you're adding a distinction between > GetPositionInIteration and GetPositionInIterationInternal when you're > not changing anything in either function except for the fact that > GetPositionInIteration doesn't pass through the last 3 parameters to > GetPositionInIterationInternal. You should undo that. > The intent here is so I can have a public and private version of GetPosition... Callers of the internal version must always call CheckNeedsRefresh afterwards. Callers of the external version don't. I thought it was risky asking clients to call CheckNeedsRefresh (and in the existing case the caller can't because they don't have an animation manager). I thought this approach was clearer than saying something like "call CheckNeedsRefresh after this if aEa and aAnimation are non-null" in a comment - that felt fragile. If you don't think it is worse, then I can do it that way. I should make the comments clearer in this case if you think it is OK to leave it as is. > > I need to think a little more about whether the CheckNeedsRefresh calls > are in sufficient places. There should be calls after every possible modification to mNeedsRefreshes, but sometimes moved up the call stack to somewhere that I have a reference to an animation manager and/or to coalesce the calls a bit.

(In reply to Nick Cameron [:nrc] from comment #45) > (In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from > comment #43) > > Comment on attachment 765869 [details] [diff] [review] > > be more precise about removing the animation manager from the refresh driver > > I think the removal of the ElementDataRemoved call from > > FlushTransitions was a mistake -- that breaks exactly this > > in the transition manager. > > > > I think this is ok because ElementDataRemoved gets called from the > destructor of CommonElementAnimationData. I think I observe this behaviour. > Am I missing something? Ah, indeed. I missed that. > > I don't see why you're adding a distinction between > > GetPositionInIteration and GetPositionInIterationInternal when you're > > not changing anything in either function except for the fact that > > GetPositionInIteration doesn't pass through the last 3 parameters to > > GetPositionInIterationInternal. You should undo that. > > > The intent here is so I can have a public and private version of > GetPosition... Callers of the internal version must always call > CheckNeedsRefresh afterwards. Callers of the external version don't. I > thought it was risky asking clients to call CheckNeedsRefresh (and in the > existing case the caller can't because they don't have an animation > manager). I thought this approach was clearer than saying something like > "call CheckNeedsRefresh after this if aEa and aAnimation are non-null" in a > comment - that felt fragile. If you don't think it is worse, then I can do > it that way. I should make the comments clearer in this case if you think it > is OK to leave it as is. I need to look into this more, but I'm still skeptical. > > I need to think a little more about whether the CheckNeedsRefresh calls > > are in sufficient places. > > There should be calls after every possible modification to mNeedsRefreshes, > but sometimes moved up the call stack to somewhere that I have a reference > to an animation manager and/or to coalesce the calls a bit. That sounds sufficient (given the calls to ElementDataRemoved that should cover the removal cases).

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #46) > (In reply to Nick Cameron [:nrc] from comment #45) > > (In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from > > comment #43) > > > Comment on attachment 765869 [details] [diff] [review] > > > I don't see why you're adding a distinction between > > > GetPositionInIteration and GetPositionInIterationInternal when you're > > > not changing anything in either function except for the fact that > > > GetPositionInIteration doesn't pass through the last 3 parameters to > > > GetPositionInIterationInternal. You should undo that. > > > > > The intent here is so I can have a public and private version of > > GetPosition... Callers of the internal version must always call > > CheckNeedsRefresh afterwards. Callers of the external version don't. I > > thought it was risky asking clients to call CheckNeedsRefresh (and in the > > existing case the caller can't because they don't have an animation > > manager). I thought this approach was clearer than saying something like > > "call CheckNeedsRefresh after this if aEa and aAnimation are non-null" in a > > comment - that felt fragile. If you don't think it is worse, then I can do > > it that way. I should make the comments clearer in this case if you think it > > is OK to leave it as is. > > I need to look into this more, but I'm still skeptical. I still think it's unnecessary complexity -- I think you should just document that callers that pass non-null values for the last 3 parameters (that is, all callers except OMTA) are responsible for calling CheckNeedsRefreshes.

(BTW, I expect to mark review+ on a patch with those issues fixed, in case that wasn't clear.)

Comment on attachment 767037 [details] [diff] [review] patch with review comments addressed >diff --git a/layout/style/AnimationCommon.h b/layout/style/AnimationCommon.h >- ~CommonElementAnimationData() >+ virtual ~CommonElementAnimationData() > { > NS_ABORT_IF_FALSE(mCalledPropertyDtor, > "must call destructor through element property dtor"); > MOZ_COUNT_DTOR(CommonElementAnimationData); > PR_REMOVE_LINK(this); > mManager->ElementDataRemoved(); > } I'd prefer not to do this; it's unnecessary virtual function call overhead. The derived classes of CommonElementAnimationData are deleted only through the property destructors (as as this code asserts). I'm guessing you did this to fix a compiler warning, but I think the right way to fix such a warning is something more like: * make ElementTransitions and ElementAnimations MOZ_FINAL (**already done**) * make ~CommonElementAnimationData protected * if needed, add explicit empty destructors for ElementTransitions and ElementAnimations, marked public (or, alternately, make ElementTransitionsPropertyDtor and ElementAnimationsPropertyDtor friends) For now, could you at least undo this change? >diff --git a/layout/style/nsAnimationManager.h b/layout/style/nsAnimationManager.h >@@ -139,16 +139,18 @@ struct ElementAnimations MOZ_FINAL > // that if the animation has not started yet, has already ended, or is paused, > // it should not be run from the compositor. When this function is called > // from the main thread, we need the actual ElementAnimation* in order to > // get correct animation-fill behavior and to fire animation events. > // This function returns -1 for the position if the animation should not be > // run (because it is not currently active and has no fill behavior), but > // only does so if aAnimation is non-null; with a null aAnimation it is an > // error to give aCurrentTime < aStartTime, and fill-forwards is assumed. >+ // GetPositionInIteration with non-null aAnimation and aEa be sure to call >+ // CheckNeedsRefresh on the animation manager afterwards. > static double GetPositionInIteration(TimeDuration aElapsedDuration, > TimeDuration aIterationDuration, > double aIterationCount, > uint32_t aDirection, > ElementAnimation* aAnimation = nullptr, > ElementAnimations* aEa = nullptr, > EventArray* aEventsToDispatch = nullptr); > How about putting "After calling" at the beginning of your addition, and a comma before the "be sure to"? r=dbaron with that

carrying r=dbaron

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #50) > Comment on attachment 767037 [details] [diff] [review] > patch with review comments addressed > > >diff --git a/layout/style/AnimationCommon.h b/layout/style/AnimationCommon.h > > >- ~CommonElementAnimationData() > >+ virtual ~CommonElementAnimationData() > > { > > NS_ABORT_IF_FALSE(mCalledPropertyDtor, > > "must call destructor through element property dtor"); > > MOZ_COUNT_DTOR(CommonElementAnimationData); > > PR_REMOVE_LINK(this); > > mManager->ElementDataRemoved(); > > } > > I'd prefer not to do this; it's unnecessary virtual function call overhead. > > The derived classes of CommonElementAnimationData are deleted only through > the property destructors (as as this code asserts). I'm guessing you did > this to fix a compiler warning, but I think the right way to fix such a > warning is something more like: > * make ElementTransitions and ElementAnimations MOZ_FINAL (**already done**) > * make ~CommonElementAnimationData protected > * if needed, add explicit empty destructors for ElementTransitions and > ElementAnimations, marked public (or, alternately, make > ElementTransitionsPropertyDtor and ElementAnimationsPropertyDtor friends) > > For now, could you at least undo this change? > Sure. There was no compiler warning, it was one of my first guesses for what was going wrong and the missing virtual made me a bit twitchy. > > >diff --git a/layout/style/nsAnimationManager.h b/layout/style/nsAnimationManager.h > >@@ -139,16 +139,18 @@ struct ElementAnimations MOZ_FINAL > > // that if the animation has not started yet, has already ended, or is paused, > > // it should not be run from the compositor. When this function is called > > // from the main thread, we need the actual ElementAnimation* in order to > > // get correct animation-fill behavior and to fire animation events. > > // This function returns -1 for the position if the animation should not be > > // run (because it is not currently active and has no fill behavior), but > > // only does so if aAnimation is non-null; with a null aAnimation it is an > > // error to give aCurrentTime < aStartTime, and fill-forwards is assumed. > >+ // GetPositionInIteration with non-null aAnimation and aEa be sure to call > >+ // CheckNeedsRefresh on the animation manager afterwards. > > static double GetPositionInIteration(TimeDuration aElapsedDuration, > > TimeDuration aIterationDuration, > > double aIterationCount, > > uint32_t aDirection, > > ElementAnimation* aAnimation = nullptr, > > ElementAnimations* aEa = nullptr, > > EventArray* aEventsToDispatch = nullptr); > > > > How about putting "After calling" at the beginning of your addition, and a > comma before the "be sure to"? > > Yes. > > > r=dbaron with that

hmm, which b2g tree should I land this too? And do I need approval?

You want to land this on 1.1, and 1.1HD. Its a blocker so no approval needed. Great work by the way. ++nrc

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/pushloghtml looks like it takes regular merges from 1.1, though.

(In other words, I think landing on https://hg.mozilla.org/releases/mozilla-b2g18 is sufficient per rules at https://wiki.mozilla.org/Release_Management/B2G_Landing#Landing_leo.2B_bugs_for_v1.1.0_.28updated_2.2F13.29 and the merge will take care of HD.)

(In reply to Nick Cameron [:nrc] from comment #53) > https://hg.mozilla.org/integration/mozilla-inbound/rev/28251f4140f6 This change seems to improve sunspider & kraken performances on awfy (B2G's browser). I am not sure how this is related to this patch, is that expected? You can find a copy of sunspider[1] and kraken[2] benchmarks in my people account. [1] http://people.mozilla.com/~npierron/sunspider/hosted/ (click on start now) [2] http://people.mozilla.com/~npierron/kraken/hosted/ (click on begin)

b2g18: https://hg.mozilla.org/releases/mozilla-b2g18/rev/fc88252e24e2