1017914 - `TypeError: this.mInput is null autocomplete.xml` error from GitHub login form in e10s window

Status: RESOLVED FIXED

(Toolkit :: Autocomplete, defect)

Description

[Chris Peterson [:cpeterson]]

When I try to log into GitHub from an e10s window, I see the following JS error in Firefox's Browser Console (not the page's Web Console):

> TypeError: this.mInput is null autocomplete.xml:898

This error does not happen in a non-e10s window.

Comment 1

[atifrea]

Attachment: The attached changes to file autocomplete.xml fix the bug.

Basically, all I do is null-check mInput or the future value of mInput.

Comment 2

[Chris Peterson [:cpeterson]]

Comment on attachment 8444787 [details] [diff] [review]
The attached changes to file autocomplete.xml fix the bug.

hi Alexandru, to request a code review, you should attach a patch, not the changed file, and then set the review flag to "?" and enter an appropriate reviewer's name.

This MDN article is a good introduction to creating a patch and requesting a code review:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch

Comment 3

[atifrea]

Attachment: Fixed the bug by null-checking mInput whenever it is used. r=felipe

Comment 5

[:Felipe Gomes (needinfo for replies!)]

I'll get to this review soon.. I had a note about this .mInput being null when I worked on the select dropdowns bug. I need to find it to see what it was about :)  (i.e., if there's something special to consider here than just doing a null check)

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8445217 [details] [diff] [review]
Fixed the bug by null-checking mInput whenever it is used. r=felipe

Apologies for the delay.. On my notes I found that I left the warning of .mInput being null to remind us to look into it more. Basically there are some things in autocomplete.xml that depends on properties of mInput, so we should check if those are missing features or not, before

The 4 things I see in autocomplete.xml that look relevant are

- showCommentColumn
- showImageColumn

I think those are not part of the web spec (only used in our UI code). With a double check to this, I think we can safely ignore those for out-of-process content and it should remain working in our UI (e.g. the searchbar)

- maxRows

this might be part of some html5 attribute that is used to customize this value

- controller

This looks like the hardest part. There are two different controllers at play, one is mController which is created in the constructor (and is not related to mInput), and the other is mInput.controller. A lot of things are accessed directly or indirectly through the controller. I suspect most things are already properly working, but I think what should be the goal for this bug is to investigate these 4 things and see what is missing, and what can be safely ignored and wrapped with a null-check.

Comment 7

[atifrea]

Attachment: Bug 1017914 - Patch v2: Null-checked mInput only in |popupshowing| and |popuphiding|. r=felipe

I have investigated the issues that you had suggested.
- this.mInput is used to assign showCommentColumn and showImageColumn new values in | _openAutocompletePopup |, but that method is never called in the child where mInput is null;
- if this.mInput is null, then the default value for maxRows is used (which is this.defaultMaxRows);
- this.mInput.controller is used only in _appendCurrentResult, view and _matchCount. As far as I can tell, neither of them is called in the child where mInput is null. They are only called in the parent (e.g. when autocompleting the URL bar).

For these reasons, I think that null-checking just in popupshowing and popuphiding shuold be enough. I hope I didn't overlook anything.

Comment 8

[:Felipe Gomes (needinfo for replies!)]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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