Remove CPU wake lock control from ContentParent

RESOLVED WONTFIX

Status

()

Core
DOM: Content Processes
RESOLVED WONTFIX
5 years ago
9 months ago

People

(Reporter: Gene Lian (I already quit Mozilla), Unassigned)

Tracking

unspecified
mozilla34
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 9 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Updated

5 years ago
Assignee: nobody → gene.lian

Updated

5 years ago
Whiteboard: RN5/29

Updated

5 years ago
Whiteboard: RN5/29
(Reporter)

Updated

4 years ago
Blocks: 755245
Depends on: 988787
Created attachment 8442021 [details] [diff] [review]
Patch v1
Assignee: gene.lian → selin
Attachment #8442021 - Flags: review?(gene.lian)
(Reporter)

Comment 2

4 years ago
Looks great but I remembered there is one test that needs to be fixed.
Created attachment 8442717 [details] [diff] [review]
Patch v2
Created attachment 8442734 [details] [diff] [review]
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
Attachment #8442021 - Attachment is obsolete: true
Attachment #8442717 - Attachment is obsolete: true
Attachment #8442021 - Flags: review?(gene.lian)
Attachment #8442734 - Flags: review?(gene.lian)
Created attachment 8442741 [details] [diff] [review]
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.)
Attachment #8442734 - Attachment is obsolete: true
Attachment #8442734 - Flags: review?(gene.lian)
Attachment #8442741 - Flags: review?(gene.lian)
(Reporter)

Comment 6

4 years ago
Awesome! I'll look into the details early next Monday. I'm going to fly.
(Reporter)

Comment 7

4 years ago
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.
Attachment #8442741 - Flags: review?(gene.lian) → review-
Created attachment 8445017 [details] [diff] [review]
Patch v4

Removing CPU wake lock control from ContentParent. But still keeping and adding relevant observers to maintain the current priority behaviors.
Attachment #8442741 - Attachment is obsolete: true
Attachment #8445017 - Flags: review?(gene.lian)
Try runs for this patch. FYI.
https://tbpl.mozilla.org/?tree=Try&rev=0fb174c593da
https://tbpl.mozilla.org/?tree=Try&rev=21831eb33be8
(Reporter)

Comment 10

3 years ago
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/
Attachment #8445017 - Flags: review?(gene.lian) → review-
Created attachment 8448585 [details] [diff] [review]
Patch v5

Maintain the logic of process priority without adding extra observers.
Attachment #8445017 - Attachment is obsolete: true
Attachment #8448585 - Flags: review?(gene.lian)
Summary: Remove SystemMessageHandledListener in ContentParent → Remove CPU wake lock control from ContentParent
(Reporter)

Comment 12

3 years ago
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->".
Attachment #8448585 - Flags: review?(gene.lian)
Created attachment 8449143 [details] [diff] [review]
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.)
Attachment #8449143 - Flags: review?(gene.lian)
(Reporter)

Comment 14

3 years ago
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).
Attachment #8448585 - Flags: review+
(Reporter)

Comment 15

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

Yeap! Thank you!
Attachment #8449143 - Flags: review?(gene.lian)
Attachment #8448585 - Flags: review?(khuey)
Component: General → DOM: Content Processes
Product: Firefox OS → Core
Attachment #8448585 - Flags: review?(khuey) → review+
Created attachment 8456608 [details] [diff] [review]
Patch v6

Merging the r+'d patch with the latest code base.
Attachment #8448585 - Attachment is obsolete: true
Attachment #8449143 - Attachment is obsolete: true
Recent try runs for this patch. FYI.
https://tbpl.mozilla.org/?tree=Try&rev=c05b8bdceeca
https://tbpl.mozilla.org/?tree=Try&rev=2778a035d80e
Keywords: checkin-needed
(Reporter)

Comment 18

3 years ago
https://hg.mozilla.org/integration/b2g-inbound/rev/3ab7869b4c47
Keywords: checkin-needed
(Reporter)

Comment 19

3 years ago
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!
Created attachment 8456748 [details] [diff] [review]
Patch v6 (reviewers appended)

Adding r=gene, khuey. Thanks for the thoroughness.
Attachment #8456608 - Attachment is obsolete: true
Keywords: checkin-needed
(Reporter)

Comment 21

3 years ago
https://hg.mozilla.org/integration/b2g-inbound/rev/5098feb565e6
Keywords: checkin-needed
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)
Created attachment 8458584 [details] [diff] [review]
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
Attachment #8456748 - Attachment is obsolete: true
Attachment #8458584 - Flags: review?(khuey)
The try run for the latest patch.
https://tbpl.mozilla.org/?tree=Try&rev=97f12e88e490
Attachment #8458584 - Flags: review?(khuey) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/957ab0b9afb2
Keywords: checkin-needed
Depends on: 1046033
https://hg.mozilla.org/mozilla-central/rev/957ab0b9afb2
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34

Updated

3 years ago
Depends on: 1046956
https://hg.mozilla.org/mozilla-central/rev/ff1e09666c42

Backed out on suspicion of causing bug 1046956.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Suspicion of causing bug 1046967 too.

Updated

3 years ago
Depends on: 1046967
Depends on: 1048111
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.
(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.
Assignee: selin → nobody
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago9 months ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.