1175024 - Don't destroy and recreate DOMRequestIpcHelper instance for every InputContext

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

According to profile at bug 970093 comment 75 we spent unnecessary amount of time (around ~10ms) in initDOMRequestHelper(). I think we should associate the helper to the message receiving window and don't recreate it for the entire life cycle of keyboard app.

I have an untested patch ready.

Comment 1

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Patch v1.0

Let's wait for Try to finish before setting the review flag.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6972dc64ab6

So in this patch I created a new interface called InputContextDOMRequestIpcHelper and make it reusable for each window. Since we already have an WindowMap we will store the instance there.

Each time InputContext created or destroyed, it would have to attach or detach itself from the InputContextDOMRequestIpcHelper.

Almost everything in InputContextDOMRequestIpcHelper are moved from InputContext, except call to destroyDOMRequestHelper() -- we will never destroy our created helper in hope of future re-usability. The helper itself has inner-window-destroyed observer attached so I presume we won't leak when the window is destroyed (also because of the fact WeakMap will not hold a strong reference to the instance).

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

The patch does not work because it changes the unset kbID from null to undefined, and I have to change the code with |if (kbID != null)| too.

There is another problem with window and maybe bfcache; I every test passes but they fail to run if I run the mochitest in batch. I have narrow down the cause to be invalid reuse of IpcHelper when we navigating away from the window, but I keep getting the same IpcHelper in the next test when I unset it upon destroy. Maybe some interesting timing involved here.

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Patch v1.1 Part 2

The previously mentioned issue was a silly mistake. I didn't unset the IpcHelper for the previous inner window correctly. This should work.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=46c9cb0ed2dd

Fabrice, unfortunate people who worked on bug 905573 and bug 1043828 have both left the project. As the reviewer of bug 905573, could you review this patch?

Olli, Jan, I am setting you for feedback just to make sure you agree with the change. I also don't know if Fabrice thinks it's better for Olli or Jan to review the patch.

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Patch v1.1 Part 1

I forget I split the patch into two parts during the investigation yesterday. This is the first part.

I will squish the two together when asking for check-in.

Comment 5

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 8624106 [details] [diff] [review]
Patch v1.1 Part 2

Sorry, I don't feel I have enough knowledge of this part of Gecko to give feedback on the patch.

Comment 6

[[:fabrice] Fabrice Desré]

Comment on attachment 8624107 [details] [diff] [review]
Patch v1.1 Part 1

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

r=me with nits addressed.

::: dom/inputmethod/Keyboard.jsm
@@ +197,5 @@
>      if (0 === msg.name.indexOf('Keyboard:') &&
>          ('Keyboard:Register' !== msg.name && this._keyboardID !== kbID)
>         ) {
> +      dump("Keyboard message " + msg.name +
> +        " from an non-active keyboard ID.");

either remove that before landing, or add a debug flag to turn it on.

::: dom/inputmethod/MozKeyboard.js
@@ +26,5 @@
>    // WeakMap of <window, object> pairs.
>    _map: null,
>  
>    /*
> +   * Set the obj associated to the window and return it

nit: s/obj/object and add a full stop at the end of the comment.

::: dom/inputmethod/mochitest/test_bug1043828.html
@@ +123,5 @@
>    }
>  
>    // STEP 2: Set keyboard A active
>    function step2() {
> +    ok(true, 'step2');

here and later, use info(X) instead of ok(true, X).

Comment 7

[[:fabrice] Fabrice Desré]

Comment on attachment 8624106 [details] [diff] [review]
Patch v1.1 Part 2

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

::: dom/inputmethod/MozKeyboard.js
@@ +468,5 @@
> +InputContextDOMRequestIpcHelper.prototype = {
> +  __proto__: DOMRequestIpcHelper.prototype,
> +  _inputContext: null,
> +
> +  attachInputContext: function ich_attachInputContext(inputCtx) {

no need to name function anymore.

@@ +484,5 @@
> +
> +  detachInputContext: function ich_detachInputContext() {
> +    // All requests that are still pending need to be invalidated
> +    // because the context is no longer valid.
> +    this.forEachPromiseResolver(function(k) {

use this.forEachPromiseResolver(k => {... to not have to bind(this)

@@ +486,5 @@
> +    // All requests that are still pending need to be invalidated
> +    // because the context is no longer valid.
> +    this.forEachPromiseResolver(function(k) {
> +      this.takePromiseResolver(k).reject("InputContext got destroyed");
> +      this.removePromiseResolver(k);

no need to remove the promise resolver since takePromiseResolver() does it for you.

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Patch to commit

review comments addressed and two patches merged.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2b9ba5aef08

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd94f7a2b847

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cd94f7a2b847