1011493 - The process priority manager does not recognize the keyboard app anymore

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Gabriele Svelto [:gsvelto]]

+++ This bug was initially created as a clone of Bug #994518 +++

While investigating bug 989713 I noticed that on master the keyboard application is always given PROCESS_PRIORITY_FOREGROUND priority when visible instead of PROCESS_PRIORITY_FOREGROUND_KEYBOARD.

This was caused by either bug 928270 or bug 915570 which changed the application type and thus prevented the process priority manager from recognizing the keyboard app. The fix is fortunately straightforward and requires changing this check to match with "inputmethod" rather than "keyboard":

http://hg.mozilla.org/mozilla-central/file/b5b35ec88db8/dom/ipc/ProcessPriorityManager.cpp#l985

Comment 1

[Gabriele Svelto [:gsvelto]]

Whoops, wrong title.

Comment 2

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

Can we add a test for this?

Comment 3

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH WIP] Adjust the process priority manager to recognize the keyboard app correctly

WIP patch with additional tests covering this issue. Somehow the priority right after the app has been created is still wrong, I have to do some further testing to figure out why.

Comment 4

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

(In reply to Gabriele Svelto [:gsvelto] from comment #3)
> WIP patch with additional tests covering this issue. Somehow the priority
> right after the app has been created is still wrong, I have to do some
> further testing to figure out why.

IIRC there's some sort of delay involved when switching priorities.

Comment 5

[Gabriele Svelto [:gsvelto]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #4)
> IIRC there's some sort of delay involved when switching priorities.

I just found out that this was caused by a code-path where we blindly set the new process priority to FOREGROUND instead of computing it when constructing a ContentParent:

http://hg.mozilla.org/mozilla-central/file/41a54c8add09/dom/ipc/ContentParent.cpp#l702

After fixing this too my new test passes correctly. I still have to do some testing on the device but we're almost there.

Comment 6

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH] Assign the correct priority to the keyboard app

Here's a patch that attempts to fix the issue: the changes involve both updating the mozapptype the process priority manager needs to look for to identify the keyboard app as well as changes in the ContentParent class to give the proper priority to the keyboard app when it's starting up.

I've added a new mochitest to check for the keyboard priority. Alternatively this test can be folded into one of the existing tests; I know we're paying attention to not making the whole testing run too long but I'm not sure how much overhead mochitests have and if it's worth avoiding to have too many of them.

Comment 7

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

Comment on attachment 8424794 [details] [diff] [review]
[PATCH] Assign the correct priority to the keyboard app

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

::: dom/ipc/ProcessPriorityManager.cpp
@@ +981,5 @@
>      }
>    }
>  
>    if (isVisible) {
> +    return HasAppType("inputmethod") ?

:P

Comment 8

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH] Assign the correct priority to the keyboard app r=khuey

Thanks for the review Kyle! The patch had bit-rotted so here's a refreshed version, I'm carrying the r+ over from comment 7. The try run is here:

https://tbpl.mozilla.org/?tree=Try&rev=015ce33c7e1d

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/50c18a716585

Comment 10

[ying.xu]

Hi,gsvelto.

Does that mean keyboard app has the priority PROCESS_PRIORITY_FOREGROUND?

and can not get killed?

tarako v1.3t has the same problem.

Comment 11

[[:fabrice] Fabrice Desré]

(In reply to ying.xu from comment #10)
> Hi,gsvelto.
> 
> Does that mean keyboard app has the priority PROCESS_PRIORITY_FOREGROUND?
> 
> and can not get killed?
> 
> tarako v1.3t has the same problem.

Tarako runs the keyboard in-process, so none of that is revelant for 1.3t.

Comment 12

[ying.xu]

(In reply to < away until June 17 > from comment #11)
> (In reply to ying.xu from comment #10)
> > Hi,gsvelto.
> > 
> > Does that mean keyboard app has the priority PROCESS_PRIORITY_FOREGROUND?
> > 
> > and can not get killed?
> > 
> > tarako v1.3t has the same problem.
> 
> Tarako runs the keyboard in-process, so none of that is revelant for 1.3t.

thanks

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/50c18a716585