874353 - Remove CPU wake lock control from ContentParent

Status: RESOLVED WONTFIX

(Core :: DOM: Content Processes, defect)

Description

[Gene Lian (I already quit Mozilla)]

After bug 866366 lands, we need to remove SystemMessageHandledListener in the ContentParent done at bug 836654, because we decided to acquire/release the CPU wake locks in the system message level, which has more accurate time points to know when a system message is actually fired or handled.

Comment 1

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Patch v1

Comment 2

[Gene Lian (I already quit Mozilla)]

Looks great but I remembered there is one test that needs to be fixed.

Comment 3

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Patch v2

Comment 4

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Patch v2

The try run for patch v1 was ok on B2G platforms [1]; whereas it failed for some priority cases (test_ExpectingSystemMessage2.html and test_HighPriorityDowngrade2.html) on other platforms [2]. After digging into them, I realized the root cause as below:

Now the CPU wake lock in ContentParent implies more than simply system message handling. Instead it's also used as one of the criteria to determine the process priority. With its own CPU wake lock at the time when the system creates a browser or an app [3], it guarantees the process will be granted for a higher priority (BACKGROUND_PERCEIVABLE) rather than BACKGROUND [4]. The reason why it requires such priority is to prevent the process from being killed before getting system messages (bug 860799). So it's unlikely for us to simply remove the whole logic of CPU wake lock from ContentParent unless we can say for sure the process can always get a higher priority when it starts.

Therefore I made a compromise by primarily removing SystemMessageHandledObserver from ContentChild and keeping part of SystemMessageHandledListener in ContentParent. However, a tradeoff is introduced: in comparison to the current mechanism, we lose the chance to earlier unlock the CPU wake lock in ContentParent once the system message is done. So it has to wait until timeout (30 secs) and then the process priority can start downgrading. Please feel free to share your concerns if it's not tolerable.

Btw, I also slightly moves the logic of sending "SystemMessageManager:HandleMessagesDone" in function |_dispatchMessage| in SystemMessageManager.js to reduce unnecessary messages.

1.) https://tbpl.mozilla.org/?tree=Try&rev=d514e1cb1e8d
2.) https://tbpl.mozilla.org/?tree=Try&rev=0d2728760ad7
3.) http://dxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#1006
4.) http://dxr.mozilla.org/mozilla-central/source/dom/ipc/ProcessPriorityManager.cpp#993

Comment 5

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Patch v3

I just realized I didn't maintain |handleCount| well when trying to reduce "SystemMessageManager:HandleMessagesDone" messages. Fallback to its original place. (More details could be found in comment 4.)

Comment 6

[Gene Lian (I already quit Mozilla)]

Awesome! I'll look into the details early next Monday. I'm going to fly.

Comment 7

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8442741 [details] [diff] [review]
Patch v3

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

Per off-line discussion, using a timer to cancel the CPU wake lock is even worse than explicitly cancelling it. I hope to see if we could have other better alternatives:

1. Lock "high-priority" directly (instead of the "cpu" lock) at which the MaybeTakeCPUWakeLock does.

2. ParticularProcessPriorityManager.mHoldsCPUWakeLock doesn't really manage the CPU wake lock. Instead, it's just a state indicating the system message(s) are done handled or not. I.e., s/mHoldsCPUWakeLock/mHandlingSystemMessages. This solution needs to keep the original "handle-system-messages-done".

::: dom/messages/SystemMessageInternal.js
@@ +39,5 @@
>    kMaxPendingMessages = 5;
>  }
>  
> +// Limit the duration to hold the CPU wake lock.
> +let kCPULockTimeoutSec;

Let's use s/kCPULockTimeoutSec/kCpuWakeLockTimeoutSec to have a more consistent semantic within this file.

Comment 8

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Patch v4

Removing CPU wake lock control from ContentParent. But still keeping and adding relevant observers to maintain the current priority behaviors.

Comment 9

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Try runs for this patch. FYI.
https://tbpl.mozilla.org/?tree=Try&rev=0fb174c593da
https://tbpl.mozilla.org/?tree=Try&rev=21831eb33be8

Comment 10

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8445017 [details] [diff] [review]
Patch v4

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

::: dom/ipc/ContentParent.cpp
@@ +1151,5 @@
> +        if (obs) {
> +            nsRefPtr<nsHashPropertyBag> props = new nsHashPropertyBag();
> +            props->SetPropertyAsUint64(NS_LITERAL_STRING("childID"), mChildID);
> +            obs->NotifyObservers(static_cast<nsIWritablePropertyBag*>(props),
> +                                 "system-message-handled", nullptr);

Please see below. You can also call a new ProcessPriorityManager::ResetProcessPriority here. But you probably need a way to pass around the ContentParent or ChildID.

@@ +1176,5 @@
>  
>  } // anonymous namespace
>  
>  void
> +ContentParent::RequestPriorityPrivilege(uint64_t aChildID)

I don't think you need to pass the childID as the parameter. It's already a member of ContentParent.

@@ +1183,5 @@
> +    if (obs) {
> +        nsRefPtr<nsHashPropertyBag> props = new nsHashPropertyBag();
> +        props->SetPropertyAsUint64(NS_LITERAL_STRING("childID"), aChildID);
> +        obs->NotifyObservers(static_cast<nsIWritablePropertyBag*>(props),
> +                             "system-message-privileged", nullptr);

I don't like the way of notifying observer. Can you add a new static function:

ProcessPriorityManager::ResetProcessPriority(dom::ContentParent* aContentParent,
                                             bool handlingSystemMessage)

and hook it up to your original OnSystemMessagePrivileged/OnSystemMessageHandled.

::: dom/ipc/ContentParent.h
@@ +329,5 @@
> +    // it's "critical" and probably has system messages coming soon. (A CPU wake
> +    // lock may already be controlled by B2G process in SystemMessageInternal.js.)
> +    // This privilege is revoked once the message is delivered, or the grace
> +    // period is up, whichever comes first.
> +    void RequestPriorityPrivilege(uint64_t aChildID);

This is not a good function name because it's related to system message specific. I think we can just drop this function and put the logic in:

if (browserFrame && browserFrame->GetIsExpectingSystemMessage()) {
  ...
}

::: dom/ipc/ProcessPriorityManager.cpp
@@ +264,5 @@
>    void OnTabParentDestroyed(nsISupports* aSubject);
>    void OnFrameloaderVisibleChanged(nsISupports* aSubject);
>    void OnChannelConnected(nsISupports* aSubject);
> +  void OnSystemMessagePrivileged(nsISupports* aSubject);
> +  void OnSystemMessageHandled(nsISupports* aSubject);

Don't need these two handlers.

@@ +303,5 @@
>    ProcessPriority mPriority;
>    ProcessCPUPriority mCPUPriority;
>    bool mHoldsCPUWakeLock;
>    bool mHoldsHighPriorityWakeLock;
> +  bool mHandlesSystemMessage;

s/mHandlesSystemMessage/mHandlingSystemMessage/

@@ +649,5 @@
>    , mPriority(PROCESS_PRIORITY_UNKNOWN)
>    , mCPUPriority(PROCESS_CPU_PRIORITY_NORMAL)
>    , mHoldsCPUWakeLock(false)
>    , mHoldsHighPriorityWakeLock(false)
> +  , mHandlesSystemMessage(false)

s/mHandlesSystemMessage/mHandlingSystemMessage/

@@ +667,5 @@
>      os->AddObserver(this, "remote-browser-shown", /* ownsWeak */ true);
>      os->AddObserver(this, "ipc:browser-destroyed", /* ownsWeak */ true);
>      os->AddObserver(this, "frameloader-visible-changed", /* ownsWeak */ true);
> +    os->AddObserver(this, "system-message-privileged", /* ownsWeak */ true);
> +    os->AddObserver(this, "system-message-handled", /* ownsWeak */ true);

Don't need these two.

@@ +744,5 @@
>      OnTabParentDestroyed(aSubject);
>    } else if (topic.EqualsLiteral("frameloader-visible-changed")) {
>      OnFrameloaderVisibleChanged(aSubject);
> +  } else if (topic.EqualsLiteral("system-message-privileged")) {
> +    OnSystemMessagePrivileged(aSubject);

Ditto.

@@ +746,5 @@
>      OnFrameloaderVisibleChanged(aSubject);
> +  } else if (topic.EqualsLiteral("system-message-privileged")) {
> +    OnSystemMessagePrivileged(aSubject);
> +  } else if (topic.EqualsLiteral("system-message-handled")) {
> +    OnSystemMessageHandled(aSubject);

Ditto.

@@ +1006,5 @@
>  
>  ProcessPriority
>  ParticularProcessPriorityManager::ComputePriority()
>  {
> +  if ((mHandlesSystemMessage || mHoldsCPUWakeLock || mHoldsHighPriorityWakeLock) &&

s/mHandlesSystemMessage/mHandlingSystemMessage/

@@ +1027,5 @@
>        PROCESS_PRIORITY_FOREGROUND_KEYBOARD :
>        PROCESS_PRIORITY_FOREGROUND;
>    }
>  
> +  if ((mHandlesSystemMessage || mHoldsCPUWakeLock || mHoldsHighPriorityWakeLock) &&

s/mHandlesSystemMessage/mHandlingSystemMessage/

Comment 11

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Patch v5

Maintain the logic of process priority without adding extra observers.

Comment 12

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8448585 [details] [diff] [review]
Patch v5

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

Almost there but I hope to see the diff of SystemMessageHandledListener. Btw, we need another reviewer to double-review the ContentParent/ContentChild part. Unfortunately, the original designer Justin and the reviewer cjones have gone, we need to find someone else to review this part. Maybe Fabrice, Kyle or Cervantese?

::: dom/ipc/ContentParent.cpp
@@ +831,5 @@
>  }
>  
> +namespace {
> +
> +class SystemMessageHandledListener MOZ_FINAL

For the convenience of review, could you please keep the SystemMessageHandledListener at the original place. You can prepare a separate patch to do the movement.

::: dom/ipc/ProcessPriorityManager.cpp
@@ +1160,5 @@
>  
>  void
> +ParticularProcessPriorityManager::SetHandlesSystemMessage(bool aHandlesSystemMessage)
> +{
> +  this->mHandlesSystemMessage = aHandlesSystemMessage;

Don't need "this->".

Comment 13

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Patch v5 (ContentParent.cpp only, no move on SystemMessageHandledListener)

ContentParent.cpp only. No move on SystemMessageHandledListener.
(This is just a supportive patch for easier comparison. The primary changes are still in patch v5.)

Comment 14

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8448585 [details] [diff] [review]
Patch v5

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

Looks good to me. r=gene

I'd like to suggest a change: s/systemMessage/systemMessage"s"/ for all the codes because the "handle-system-messages-done" is actually fired after multiple system messages are handled. It's not a big deal but I'm glad to see it if you're available to polish it (just do this in a separate patch or bug).

Comment 15

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8449143 [details] [diff] [review]
Patch v5 (ContentParent.cpp only, no move on SystemMessageHandledListener)

Yeap! Thank you!

Comment 16

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Patch v6

Merging the r+'d patch with the latest code base.

Comment 17

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Recent try runs for this patch. FYI.
https://tbpl.mozilla.org/?tree=Try&rev=c05b8bdceeca
https://tbpl.mozilla.org/?tree=Try&rev=2778a035d80e

Comment 18

[Gene Lian (I already quit Mozilla)]

https://hg.mozilla.org/integration/b2g-inbound/rev/3ab7869b4c47

Comment 19

[Gene Lian (I already quit Mozilla)]

Sorry I need to backout https://hg.mozilla.org/integration/b2g-inbound/rev/d48107587dcb because the patch doesn't specify the reviewers. Please update the patch again. Thanks!

Comment 20

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Patch v6 (reviewers appended)

Adding r=gene, khuey. Thanks for the thoroughness.

Comment 21

[Gene Lian (I already quit Mozilla)]

https://hg.mozilla.org/integration/b2g-inbound/rev/5098feb565e6

Comment 22

[Carsten Book [:Tomcat]]

sorry had to backout this change for causing crashes on linux like https://tbpl.mozilla.org/php/getParsedLog.php?id=43925418&tree=B2g-Inbound in mochitest 2 (i guess that one is also on the try run from comment 17)

Comment 23

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Patch v7

Use ResetPriorityNow() instead of ResetPriority() upon CPU wake lock changes to work around intermittent failures in some edge cases.

A simple try run shows it works.
https://tbpl.mozilla.org/?tree=Try&rev=ab4f57d922ff

The symptom of the issue is stated below:
The previous patch sufferrs an intermittent issue where some arbitrary test cases under /tests/dom/imptests/editing/selecttest/ tend to fail when running under 32-bit Linux debug mode. [1] When we use ResetPriority(), the ParticularProcessPriorityManager schedules a timer and then starts reset the priority after timeout. [2] In some rare cases when the timer fires, somehow the returned object of ManagedPBrowserParent() of the ContentParent couldn't work well. So a segmentation fault happens when accessing that object. [3][4]

[1] https://tbpl.mozilla.org/?rev=5098feb565e6&tree=B2g-Inbound
[2] https://hg.mozilla.org/integration/b2g-inbound/file/5098feb565e6/dom/ipc/ProcessPriorityManager.cpp#l940
[3] https://tbpl.mozilla.org/php/getParsedLog.php?id=43925418&tree=B2g-Inbound#error1
[4] https://hg.mozilla.org/integration/b2g-inbound/file/5098feb565e6/dom/ipc/ProcessPriorityManager.cpp#l1001

Comment 24

[Sean Lin [:seanlin] (inactive from Mar 2016)]

The try run for the latest patch.
https://tbpl.mozilla.org/?tree=Try&rev=97f12e88e490

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/957ab0b9afb2

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/957ab0b9afb2

Comment 27

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/mozilla-central/rev/ff1e09666c42

Backed out on suspicion of causing bug 1046956.

Comment 28

[Hubert Figuiere [:hub]]

Suspicion of causing bug 1046967 too.

Comment 29

[Sean Lin [:seanlin] (inactive from Mar 2016)]

A patch in the dependent bug 1046033 provides a possible fix (still under review) for this one. I'm going to wait for it and then to see if there's still something to do here.

Comment 30

[Gabriele Svelto [:gsvelto]]

(In reply to Sean Lin [:seanlin] from comment #29)
> A patch in the dependent bug 1046033 provides a possible fix (still under
> review) for this one. I'm going to wait for it and then to see if there's
> still something to do here.

Yes, on that topic see my review comment (bug 1046033 comment 8) because it applies to the change here too: exposing ProcessPriorityManager*::ResetProcessPriority() and having it called explicitly from external code is asking for trouble. The issue is that the priority manager adjusts priorities only based on external changes which makes most of its behavior quite deterministic in spite of most changes being asynchronous. Having external code adjust priorities manually breaks this assumption making the process prone to race conditions and generally speaking more fragile.