1273719 - Startup JavaScript strict warning: NightlyDebug.app/Contents/Resources/components/nsSearchService.js: reference to undefined property json._readOnly/param.purpose

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

[Jonathan Watt [:jwatt]]

During startup I'm seeing the following adding to the clutter in the terminal debug output:

JavaScript strict warning: NightlyDebug.app/Contents/Resources/components/nsSearchService.js, line 3347: ReferenceError: reference to undefined property json._readOnly

JavaScript strict warning: NightlyDebug.app/Contents/Resources/components/nsSearchService.js, line 1200: ReferenceError: reference to undefined property param.purpose

It sure would be nice if nsSearchService.js could be fixed to handle these cases without causing more junk to clutter up the terminal output. Such noise makes it harder to figure out which debug output is relevant to real bugs that people are trying to figure out and fix.

Comment 1

[Jonathan Watt [:jwatt]]

Needinfo'ing you as discussed on IRC a little while back.

Comment 2

[Florian Quèze [:florian]]

This only happens on debug builds when the profile contains user-installed search plugins. I don't think it happens for clean/new profiles.

This noise is also visible when running some xpcshell tests in toolkit/components/search.

Comment 3

[Jonathan Watt [:jwatt]]

(In reply to Florian Quèze [:florian] [:flo] from comment #2)
> This only happens on debug builds when the profile contains user-installed
> search plugins. I don't think it happens for clean/new profiles.

As noted on IRC (a month ago, so no surprise if you don't remember ;) ):

I have 8 search providers in that build/profile. Google's the default, and the others are yahoo, bing, amazon, duckduckgo, ebay, twitter and wikipedia. So I don't think I have an user-installed search plugins...unless the provider is so broken it isn't showing up in about:preferences#search . But I don't think that's the case since I don't remember installing any search plugins.

Comment 4

[Jonathan Watt [:jwatt]]

Oh, and the "Restore Default Search Engines" button is disabled, suggesting that these are the defaults.

Comment 5

[Jonathan Watt [:jwatt]]

To be clear, it's passing |json._readOnly| or |param.purpose| off as parameters to function calls that will cause errors if the referenced property is not defined.

If the intent is that the properties are allowed to be undefined, then this can be made explicit at the function calls to fix this error. As in:

  myFunc(obj.mayNotBeDefined || undefined)

is fine.

Comment 6

[Florian Quèze [:florian]]

Here are the JS Warnings I see while running the toolkit/component/search xpcshell tests:
Count Message
    1 [JavaScript Warning: "ReferenceError: reference to undefined property engine._readOnly" {components/nsSearchService.js" line: 3362}]"
    5 [JavaScript Warning: "ReferenceError: reference to undefined property json._readOnly" {components/nsSearchService.js" line: 3377}]"
    6 [JavaScript Warning: "ReferenceError: reference to undefined property param.purpose" {components/nsSearchService.js" line: 1197}]"
    1 [JavaScript Warning: "ReferenceError: reference to undefined property url.resultDomain" {components/nsSearchService.js" line: 2057}]"

Comment 7

[Florian Quèze [:florian]]

(In reply to Jonathan Watt [:jwatt] from comment #5)

> If the intent is that the properties are allowed to be undefined, then this
> can be made explicit at the function calls to fix this error. As in:
> 
>   myFunc(obj.mayNotBeDefined || undefined)
> 
> is fine.

Be careful with this as it breaks for properties that are sometimes undefined, but sometimes a string, where "" is a valid value. That was the case here for the param.purpose warning and a test caught it, even though the case where we have an empty string is no longer valid since bug 1181645.

Comment 8

[Florian Quèze [:florian]]

Attachment: Fix

For param.purpose and url.resultDomain, I think it's fine to just silence the warning (this change would have been a problem for param.purpose as "" used to be a valid value until bug 1181645, but now it's fine, so I'm just removing this case from the test_json_cache.js test).

For _readOnly, we had a real bug, in that we were giving the wrong value to the Engine constructor. Thankfully, this didn't have any consequences, because the constructor doesn't use the aIsReadOnly parameter when the first parameter is a non-empty string.
This _readOnly property is a bit confusing, because when it's on the Engine JS object it's a boolean (true or false), but when it's in the JSON cache, it's either undefined (ie. not written to the file because true is the default value) or false. After the patch, the tests using the _readOnly value from the JSON cache correctly compare to undefined to create a boolean.

Comment 9

[Jonathan Watt [:jwatt]]

(In reply to Florian Quèze [:florian] [:flo] from comment #7)
> Be careful with this as it breaks for properties that are sometimes
> undefined, but sometimes a string, where "" is a valid value.

That's a very good point!

Thanks for working on this.

Comment 10

[Florian Quèze [:florian]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3fc910bbd56

Comment 11

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/bf3b012748a42d2af8406064c3466706c7d9f0c8
Bug 1273719 - JavaScript strict warnings in nsSearchService.js (reference to undefined property json._readOnly/param.purpose), r=adw.

Comment 12

[Carsten Book [:Tomcat]]

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

Comment 13

[Mike Kaply [:mkaply]]

https://hg.mozilla.org/integration/fx-team/rev/f4c18cbc1201927d52b23a97dad073a5617237bd
Bug 1273719 - Allow chrome/resource icon URLs for built-in search engines. r=florian

Comment 14

[Florian Quèze [:florian]]

(In reply to Mike Kaply [:mkaply] from comment #13)
> https://hg.mozilla.org/integration/fx-team/rev/
> f4c18cbc1201927d52b23a97dad073a5617237bd
> Bug 1273719 - Allow chrome/resource icon URLs for built-in search engines.
> r=florian

This was bug 1275366.

Comment 15

[Mike Kaply [:mkaply]]

https://hg.mozilla.org/integration/fx-team/rev/53c4879adb371bc05e4e0312acdda964c1ff235d
Back out changeset f4c18cbc1201 to correct wrong bug (1273719) in commit message.