Don't destroy and recreate DOMRequestIpcHelper instance for every InputContext

RESOLVED FIXED in Firefox 42

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: timdream, Assigned: timdream)

Tracking

unspecified
mozilla42
Points:
---

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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.
Created attachment 8623478 [details] [diff] [review]
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).
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.
Created attachment 8624106 [details] [diff] [review]
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.
Attachment #8623478 - Attachment is obsolete: true
Attachment #8624106 - Flags: review?(fabrice)
Attachment #8624106 - Flags: feedback?(janjongboom)
Attachment #8624106 - Flags: feedback?(bugs)
Created attachment 8624107 [details] [diff] [review]
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.
Attachment #8624107 - Flags: review?(fabrice)
Attachment #8624107 - Flags: feedback?(janjongboom)
Attachment #8624107 - Flags: feedback?(bugs)
Attachment #8624106 - Attachment description: Patch v1.1 → Patch v1.1 Part 2
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.
Attachment #8624106 - Flags: feedback?(janjongboom)
Attachment #8624107 - Flags: feedback?(janjongboom)
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).
Attachment #8624107 - Flags: review?(fabrice) → review+
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.
Attachment #8624106 - Flags: review?(fabrice) → review+
Created attachment 8629240 [details] [diff] [review]
Patch to commit

review comments addressed and two patches merged.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2b9ba5aef08
Attachment #8624106 - Attachment is obsolete: true
Attachment #8624107 - Attachment is obsolete: true
Attachment #8624106 - Flags: feedback?(bugs)
Attachment #8624107 - Flags: feedback?(bugs)
Attachment #8629240 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cd94f7a2b847
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.