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

RESOLVED FIXED in mozilla35

Status

()

Toolkit
Autocomplete
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: cpeterson, Assigned: atifrea)

Tracking

(Blocks: 1 bug, {reproducible})

unspecified
mozilla35
x86
Mac OS X
reproducible
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
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.

Updated

4 years ago
Component: Bookmarks & History → Autocomplete
Product: Firefox → Toolkit
tracking-e10s: ? → +
(Assignee)

Comment 1

4 years ago
Created attachment 8444787 [details] [diff] [review]
The attached changes to file autocomplete.xml fix the bug.

Basically, all I do is null-check mInput or the future value of mInput.
Attachment #8444787 - Flags: review+
(Reporter)

Comment 2

4 years ago
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
Attachment #8444787 - Flags: review+
(Assignee)

Comment 3

3 years ago
Created attachment 8445217 [details] [diff] [review]
Fixed the bug by null-checking mInput whenever it is used. r=felipe
(Reporter)

Updated

3 years ago
Attachment #8445217 - Flags: review?(felipc)
Assignee: nobody → atifrea
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 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.
Attachment #8445217 - Flags: review?(felipc)
(Assignee)

Updated

3 years ago
Attachment #8444787 - Attachment is obsolete: true
(Assignee)

Comment 7

3 years ago
Created attachment 8468922 [details] [diff] [review]
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.
Attachment #8445217 - Attachment is obsolete: true
Attachment #8468922 - Flags: review?(felipc)
Attachment #8468922 - Flags: review?(felipc) → review+

Updated

3 years ago
Blocks: 947503

Updated

3 years ago
Blocks: 1055753
https://hg.mozilla.org/integration/mozilla-inbound/rev/e05499b01d08
Duplicate of this bug: 947503
https://hg.mozilla.org/mozilla-central/rev/e05499b01d08
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.