Closed Bug 874353 Opened 6 years ago Closed 3 years ago

Remove CPU wake lock control from ContentParent

Categories

(Core :: DOM: Content Processes, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED WONTFIX
mozilla34

People

(Reporter: airpingu, Unassigned)

References

Details

Attachments

(1 file, 9 obsolete files)

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.
Assignee: nobody → gene.lian
Whiteboard: RN5/29
Whiteboard: RN5/29
Depends on: 988787
Attached patch Patch v1 (obsolete) — Splinter Review
Assignee: gene.lian → selin
Attachment #8442021 - Flags: review?(gene.lian)
Looks great but I remembered there is one test that needs to be fixed.
Attached patch Patch v2 (obsolete) — Splinter Review
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)
Attached patch Patch v3 (obsolete) — Splinter Review
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)
Awesome! I'll look into the details early next Monday. I'm going to fly.
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-
Attached patch Patch v4 (obsolete) — Splinter Review
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)
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-
Attached patch Patch v5 (obsolete) — Splinter Review
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
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)
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)
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+
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)
Component: General → DOM: Content Processes
Product: Firefox OS → Core
Attached patch Patch v6 (obsolete) — Splinter Review
Merging the r+'d patch with the latest code base.
Attachment #8448585 - Attachment is obsolete: true
Attachment #8449143 - Attachment is obsolete: true
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!
Attached patch Patch v6 (reviewers appended) (obsolete) — Splinter Review
Adding r=gene, khuey. Thanks for the thoroughness.
Attachment #8456608 - Attachment is obsolete: true
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)
Attached patch Patch v7Splinter Review
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)
Depends on: 1046033
https://hg.mozilla.org/mozilla-central/rev/957ab0b9afb2
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
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.
Depends on: 1046967
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
Closed: 5 years ago3 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.