1046033 - Remove the "expecting-system-message" attribute support and GetIsExpectingSystemMessage from nsIMozBrowserFrame

Status: RESOLVED INVALID

(Core :: DOM: Content Processes, defect)

Description

[Kan-Ru Chen [:kanru] (UTC+9)]

Currently gaia system app set "expecting-system-message" for all apps that have a manifest so this attribute virtually gives all apps a higher priority for a period of time. That makes me feel this attribute is not needed.

We should be able to infer from the system message internal that a process has a pending system message thus give it a higher priority. Then we don't have to expose this weird attribute and we can control the process priority entirely in the b2g process.

Comment 1

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Kan-Ru Chen [:kanru] from comment #0)
> We should be able to infer from the system message internal that a process
> has a pending system message thus give it a higher priority. Then we don't
> have to expose this weird attribute and we can control the process priority
> entirely in the b2g process.

Agreed, it would be a lot cleaner if we could get rid of this attribute. It would also be more robust since we would then be relying on the real presence of a system message thus avoiding false positives where we give higher priority to apps that aren't really waiting for one.

Comment 2

[[:fabrice] Fabrice Desré]

I agree too. We even added automatic wakelock grabs in the system message code already (see https://mxr.mozilla.org/mozilla-central/search?find=%2Fdom%2Fmessages%2F&string=wakelock).

Comment 3

[Evelyn Hung]

Attachment: bug-1046033-remove-expecting-system-message-v1.patch

(This patch is based on bug 874353)

In this patch, I removed expecting-system-message attribute and its corresponding tests. Instead, the information will be directly queried from SystemMessageInternal.jsm. The information will affect process priority when we ask to recalculate it.

I also removed 'SystemMessageHandledListener' in ContentParent, because the only thing it did was registering a timer for expiring 'expecting-system-message' priority. I move the timer into ContentParent.

Thing isn't covered in this patch is test case. I'm thinking adding them to system message module but haven't figure out how to do it.

Comment 4

[Evelyn Hung]

Attachment: gaia-system-app-pull-request

Comment 5

[Evelyn Hung]

try server result for attachment 8466779 [details] [diff] [review] (also includes the patch on bug 874353)
https://tbpl.mozilla.org/?tree=Try&rev=3507e044f433

Comment 6

[[:fabrice] Fabrice Desré]

Comment on attachment 8466779 [details] [diff] [review]
bug-1046033-remove-expecting-system-message-v1.patch

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

lgtme, but I'd like Gabriele to review, and I'd like to see new tests to replace the ones you removed.

::: dom/ipc/ContentParent.cpp
@@ +596,5 @@
>          return nullptr;
>      }
>  
> +    // set its manifest first because we need mAppManifestURL while setting
> +    // process priority

nit: start the comment with a Capital and end with a full stop.

@@ +779,5 @@
> +        do_GetService("@mozilla.org/system-message-internal;1");
> +
> +    bool result;
> +    rv = systemMessenger->HasPendingMessage(manifestURI, &result);
> +    return (rv == NS_OK && result) ?

nit: s/rv == NS_OK/NS_SUCCEEDED(rv)

@@ +1933,5 @@
> +    nsCOMPtr<nsISystemMessagesInternal> systemMessenger =
> +        do_GetService("@mozilla.org/system-message-internal;1");
> +    bool result;
> +    rv = systemMessenger->HasPendingMessage(manifestURI, &result);
> +    return (rv == NS_OK && result);

same here. Why aren't your reusing this method in Startup() ?

::: dom/ipc/ContentParent.h
@@ +172,5 @@
>      {
>        return mIsForBrowser;
>      }
> +
> +    /** Check if any pending system message via SystemMessageInter.jsm **/

nit: comment style is //

::: dom/ipc/ProcessPriorityManager.cpp
@@ +499,5 @@
>      GetParticularProcessPriorityManager(aContentParent);
>    pppm->SetPriorityNow(aPriority, aBackgroundLRU);
>  }
>  
> +//evelyn: clean up this method

Is that done?

::: dom/messages/SystemMessageInternal.js
@@ +303,5 @@
> +
> +    return this._pages.some(function(aPage) {
> +      return (aPage.manifestURL === manifestURL &&
> +              aPage.pendingMessages.length > 0);
> +    }, this);

nit: you don't need the |this| parameter here.

Comment 7

[Evelyn Hung]

Attachment: bug-1046033-remove-expecting-system-message-v2.patch

1. Fabrice's comments addressed.
2. This patch is based on latest master, I picked some necessary changes in bug 874353.

Comment 8

[Gabriele Svelto [:gsvelto] (PTO)]

Comment on attachment 8470625 [details] [diff] [review]
bug-1046033-remove-expecting-system-message-v2.patch

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

I like the general approach as I'm pretty keen on removing this unneeded complexity but there's a couple of important issues I want to see addressed before r+'ing this:

- First of all this needs test coverage, removing the existing unit-tests is not acceptable, more on this later.

- Exposing ProcessPriorityManager::ResetProcessPriority() feels wrong and I think is also unneeded. Again, more on this later.

BTW sorry for the delay but I was on PTO.

::: dom/browser-element/mochitest/priority/mochitest.ini
@@ -10,5 @@
>  [test_Visibility.html]
>  [test_HighPriority.html]
>  support-files = file_HighPriority.html
> -[test_HighPriorityDowngrade.html]
> -[test_HighPriorityDowngrade2.html]

These tests cover the fundamental transitions to/from high-priority states; these cannot be simply taken out as we'd be removing some critically important coverage from the process priority management functionality. If these tests won't work anymore with your patch you need to re-implement them somehow; this can't land without proper coverage.

@@ -21,5 @@
>  support-files = file_MultipleFrames.html
>  # This test is disabled temporarily in bug 968604. It will be enabled after bug 987164 is fixed.
>  #[test_Preallocated.html]
> -[test_ExpectingSystemMessage.html]
> -[test_ExpectingSystemMessage2.html]

Likewise.

::: dom/ipc/ContentParent.cpp
@@ +778,5 @@
> +    nsCOMPtr<nsISystemMessagesInternal> systemMessenger =
> +        do_GetService("@mozilla.org/system-message-internal;1");
> +
> +    bool result;
> +    rv = systemMessenger->HasPendingMessage(manifestURI, &result);

The code above is practically identical to ContentParent::IsExpectingSystemMessage(), why not use that instead of duplicating it?

@@ +3496,5 @@
>  
>  bool
>  ContentParent::RecvSystemMessageHandled()
>  {
> +    ProcessPriorityManager::ResetProcessPriority(this);

Do we really need this? It seems that we're already releasing the CPU wakelock when we're done with the last system message:

https://mxr.mozilla.org/mozilla-central/source/dom/messages/SystemMessageInternal.js#509

In this case the ProcessPriorityManager should already reset all task priorities:

http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ProcessPriorityManager.cpp#629

::: dom/ipc/ProcessPriorityManager.cpp
@@ +133,5 @@
>  
>    /**
> +   * This function implements ProcessPriorityManager::ResetProcessPriority.
> +   */
> +  void ResetProcessPriority(ContentParent* aContentParent);

As mentioned before, I'm not sure if we need to expose this. Locking/unlocking the cpu wakelock in the SystemMessageManager should already trigger the necessary process priority adjustments. In general I'd like to make the ProcessPriorityManger rely on objective inputs (e.g. is the application holding a wakelock? Is it in the foreground?) rather than on explicit adjustments made from the code (the application told me to adjust priorities). The former is pretty robust while the latter feels too brittle as it makes process priority rely on external code to explicitly inform the ProcessPriorityManager instead of just doing what it needs to do and having its priority automatically adjusted.

@@ +500,5 @@
>    pppm->SetPriorityNow(aPriority, aBackgroundLRU);
>  }
>  
>  void
> +ProcessPriorityManagerImpl::ResetProcessPriority(ContentParent* aContentParent)

Likewise.

@@ +1466,5 @@
>    }
>  }
>  
>  /* static */ void
> +ProcessPriorityManager::ResetProcessPriority(ContentParent* aContentParent)

Likewise.

::: dom/ipc/ProcessPriorityManager.h
@@ +68,5 @@
> +   * main process.
> +   *
> +   * The process priority manager will determine a new appropriate priority.
> +   */
> +  static void ResetProcessPriority(dom::ContentParent* aContentParent);

Likewise.