Various errors in FormHelper while running the tests in bug 891688

RESOLVED FIXED in Firefox 25

Status

Firefox for Metro
Input
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jimm, Assigned: jimm)

Tracking

Trunk
Firefox 25
x86_64
Windows 8.1

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
TEST-INFO | chrome://mochitests/content/metro/browser/metro/base/tests/mochitest/browser_selection_caretfocus.js | Console message: [JavaScript Error: "aElement.ownerDocument is null" {file: "chrome://browser/content/contenthandlers/FormHelper.js" line: 429}]

TEST-INFO | chrome://mochitests/content/metro/browser/metro/base/tests/mochitest/browser_selection_caretfocus.js | Console message: [JavaScript Error: "aElement.ownerDocument is undefined" {file: "chrome://browser/content/contenthandlers/FormHelper.js" line: 429}]

TEST-INFO | chrome://mochitests/content/metro/browser/metro/base/tests/mochitest/browser_selection_caretfocus.js | Console message: [JavaScript Error: "element is null" {file: "chrome://browser/content/contenthandlers/FormHelper.js" line: 515}]
(Assignee)

Comment 1

5 years ago
Created attachment 782523 [details] [diff] [review]
fixes
Assignee: nobody → jmathies
(Assignee)

Comment 2

5 years ago
Comment on attachment 782523 [details] [diff] [review]
fixes

Simple null element avoidance, shouldn't change functionality at all.
Attachment #782523 - Flags: review?(rsilveira)
Comment on attachment 782523 [details] [diff] [review]
fixes

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

Fix looks good. There are a couple of potential future similar issues though.

::: browser/metro/base/content/contenthandlers/FormHelper.js
@@ +82,5 @@
>        // example, in this "fun" case just do nothing if the element is hidden
> +      if (self._isVisibleElement(gFocusManager.focusedElement)) {
> +        let json = self._getJSON();
> +        if (json.current) {
> +          sendAsyncMessage("FormAssist:Show", json);

There are other msgs being sent with _getJSON like https://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/contenthandlers/FormHelper.js#207
and https://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/contenthandlers/FormHelper.js#285
Do we need similar guards there too?

@@ +513,5 @@
>  
>    _getJSON: function() {
>      let element = this.currentElement;
> +    if (!element) {
> +      return {};

FormHelperUI._onShowRequest expects current.choices to be present in the JSON https://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/helperui/FormHelperUI.js#100
If we keep the unguarded msgs, this is a potential issue.
Attachment #782523 - Flags: review?(rsilveira) → review+
(Assignee)

Comment 4

5 years ago
Ah good catch, didn't think to look at other callers of _getJSON. Let me take a look, will post a follow up as needed.
(Assignee)

Comment 5

5 years ago
Created attachment 783100 [details] [diff] [review]
part 2 - wrap json msg sending

This should do it.
Attachment #783100 - Flags: review?(rsilveira)
Comment on attachment 783100 [details] [diff] [review]
part 2 - wrap json msg sending

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

Looks great!
Attachment #783100 - Flags: review?(rsilveira) → review+
https://hg.mozilla.org/mozilla-central/rev/59f13ddeb055
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25

Comment 9

5 years ago
Please can bugs changing these parts of the tree land on fx-team in the future to help balance load & avoid conflicts (for more info see Gavin's newsgroup post: https://mail.mozilla.org/pipermail/firefox-dev/2013-July/000618.html). Thank you :-)
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.