Strict errors during test: localName can be undefined

RESOLVED FIXED in Firefox 63

Status

()

P5
normal
RESOLVED FIXED
5 months ago
5 months ago

People

(Reporter: mkaply, Assigned: dpino)

Tracking

({good-first-bug})

Trunk
Firefox 63
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

5 months ago
I'm seeing strict errors in running some tests:

pid:5928 JavaScript strict warning: file:///Users/michaelkaply/Projects/mozilla-unified/obj-x86_64-apple-darwin16.7.0/dist/Nightly.app/Contents/Resources/components/nsSearchService.js, line 1870: ReferenceError: reference to undefined property "localName"
 0:01.17 pid:5928 JavaScript strict warning: file:///Users/michaelkaply/Projects/mozilla-unified/obj-x86_64-apple-darwin16.7.0/dist/Nightly.app/Contents/Resources/components/nsSearchService.js, line 1789: ReferenceError: reference to undefined property "localName"
 0:01.17 INFO "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "ReferenceError: reference to undefined property "localName"" {file: "file:///Users/michaelkaply/Projects/mozilla-unified/obj-x86_64-apple-darwin16.7.0/dist/Nightly.app/Contents/Resources/components/nsSearchService.js" line: 1870}]"
 0:01.17 INFO "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "ReferenceError: reference to undefined property "localName"" {file: "file:///Users/michaelkaply/Projects/mozilla-unified/obj-x86_64-apple-darwin16.7.0/dist/Nightly.app/Contents/Resources/components/nsSearchService.js" line: 1789}]"

We're assuming localName in certain places in the code, but #text nodes don't have a localName.
(Assignee)

Comment 1

5 months ago
I attempted to fix this bug, but I didn't manage to reproduce it. Could you provide more info about what tests to run to reproduce it?
Flags: needinfo?(mozilla)
(Reporter)

Comment 2

5 months ago
Sure.

See:

https://bugzilla.mozilla.org/show_bug.cgi?id=1482085

toolkit/components/places/tests/unifiedcomplete/test_PlacesSearchAutocompleteProvider.js 

should show it.

Thanks!
Flags: needinfo?(mozilla)

Comment 3

5 months ago
Yes, run
  ./mach xpcshell-test toolkit/components/places/tests/unifiedcomplete/test_PlacesSearchAutocompleteProvider.js
and watch the output.

Text nodes don't have an attribute 'localName' so I think this should be changed to 'nodeName'.
(Assignee)

Comment 4

5 months ago
Created attachment 9000171 [details] [diff] [review]
Skip text nodes when iterating document nodes

Thank you very much for the pointers!

So the main reason for this error was that when iterating the document's nodes (i.e searchplugins/google.xml) for comparing nodes against a tag name ("Param", "MozParam", "ShortName", etc), the iteration was done using 'childNodes', which returns all nodes of a document (including text nodes) [1]. If I recall correctly, an element node always includes a text node, which explains where the text nodes were coming from.

So I changed the function 'childNodes' for 'children' which returns only element nodes. In addition, I replaced all references of 'localName' for 'nodeName', since 'localName' is obsolete according to [2].

[1] https://developer.mozilla.org/en-US/docs/Web/API/Node/childNodes
[2] https://developer.mozilla.org/en-US/docs/Web/API/Node/localName
Attachment #9000171 - Flags: review?(adw)
Comment on attachment 9000171 [details] [diff] [review]
Skip text nodes when iterating document nodes

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

Thanks!  This works, and I hate to ask you to change it, but while you're right that localName is obsolete on Node, it's not obsolete on Element: https://developer.mozilla.org/en-US/docs/Web/API/Element/localName  We could switch to nodeName, but I don't see a good reason to do that, so would you mind reverting those lines?  (_data is set to documentElement, so even in that case, localName is OK.)  Switching from childNodes to children is all that's needed to fix this -- and thanks for catching that.
Attachment #9000171 - Flags: review?(adw)

Updated

5 months ago
Assignee: nobody → dpino
Status: NEW → ASSIGNED
(Assignee)

Comment 6

5 months ago
Created attachment 9003032 [details] [diff] [review]
0001-Skip-text-nodes-when-iterating-document-nodes.patch

I agree if in fact there's no justified reason to change localName for nodeName, it makes to leave it as it is. Patch updated.
Attachment #9000171 - Attachment is obsolete: true
(Assignee)

Comment 7

5 months ago
Created attachment 9003062 [details] [diff] [review]
Bug-1482230-Skip-text-nodes-when-iterating-document-.patch

Adjust commit message to right format.
Attachment #9003032 - Attachment is obsolete: true
Attachment #9003062 - Flags: review?(adw)
Comment on attachment 9003062 [details] [diff] [review]
Bug-1482230-Skip-text-nodes-when-iterating-document-.patch

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

Thanks!
Attachment #9003062 - Flags: review?(adw) → review+

Comment 9

5 months ago
Could you please submit a patch without the

---
 toolkit/components/search/nsSearchService.js | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

This text would be committed to our code base.

The two lines above
When iterating document's nodes, use 'children' instead of 'childNodes'
to retrieve element nodes only.
are OK.

When you do, please change the commit message to:
Bug 1482230 - Skip text nodes when iterating document nodes. r=adw.

Thank you.

Comment 10

5 months ago
Sorry, without the trailing fullstop, so:
Bug 1482230 - Skip text nodes when iterating document nodes. r=adw
(Assignee)

Comment 11

5 months ago
Created attachment 9003303 [details] [diff] [review]
Bug-1482230-Skip-text-nodes-when-iterating-document-.patch

Patch updated.
Attachment #9003062 - Attachment is obsolete: true

Comment 12

5 months ago
Comment on attachment 9003303 [details] [diff] [review]
Bug-1482230-Skip-text-nodes-when-iterating-document-.patch

You can simply say:
Added reviewer to commit message, carrying forward Drew's (:adw) r+ :-)
Attachment #9003303 - Flags: review+

Updated

5 months ago
Keywords: checkin-needed

Comment 13

5 months ago
Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/394f359ada2d
Skip text nodes when iterating document nodes. r=adw
Keywords: checkin-needed

Comment 14

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/394f359ada2d
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox63: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
You need to log in before you can comment on or make changes to this bug.