Closed Bug 1482230 Opened 2 years ago Closed 2 years ago

Strict errors during test: localName can be undefined

Categories

(Firefox :: Search, defect, P5)

defect

Tracking

()

RESOLVED FIXED
Firefox 63
Tracking Status
firefox63 --- fixed

People

(Reporter: mkaply, Assigned: dpino)

Details

(Keywords: good-first-bug)

Attachments

(1 file, 3 obsolete files)

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.
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)
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)
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'.
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)
Assignee: nobody → dpino
Status: NEW → ASSIGNED
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
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+
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.
Sorry, without the trailing fullstop, so:
Bug 1482230 - Skip text nodes when iterating document nodes. r=adw
Patch updated.
Attachment #9003062 - Attachment is obsolete: true
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+
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
https://hg.mozilla.org/mozilla-central/rev/394f359ada2d
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
You need to log in before you can comment on or make changes to this bug.