Gabriele Svelto [:gsvelto] Assignee
	Description • 11 years ago

+++ 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

	Gabriele Svelto [:gsvelto] Assignee
	Comment 1 • 11 years ago

Whoops, wrong title.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 2 • 11 years ago

Can we add a test for this?

	Gabriele Svelto [:gsvelto] Assignee
	Comment 3 • 11 years ago

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.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 4 • 11 years ago

(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.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 5 • 11 years ago

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 6 • 11 years ago

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.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 7 • 10 years ago

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

	Gabriele Svelto [:gsvelto] Assignee
	Comment 8 • 10 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 9 • 10 years ago

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

	ying.xu
	Comment 10 • 10 years ago

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.

	[:fabrice] Fabrice Desré
	Comment 11 • 10 years ago

(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.

	ying.xu
	Comment 12 • 10 years ago

(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

	Phil Ringnalda (:philor)
	Comment 13 • 10 years ago

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