Closed Bug 1455261 Opened Last year Closed Last year

Error: [Exception... "[JavaScript Error: "this._metaData is undefined" {file: "jar:file:///C:/Program%20...

Categories

(Firefox :: Search, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 62
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 61+ verified
firefox60 --- wontfix
firefox61 + verified
firefox62 + verified

People

(Reporter: standard8, Assigned: mkaply)

References

()

Details

(Whiteboard: [nightly-js-sentry:3420182])

Attachments

(1 file)

This bug was automatically filed from Sentry: https://sentry.prod.mozaws.net/operations/nightly-js-errors/issues/3420182/

Error: [Exception... "[JavaScript Error: "this._metaData is undefined" {file: "jar:file:///C:/Program%20Files/Nightly/omni.ja!/components/nsSearchService.js" line: 2080}]'[JavaScript Error: "this._metaData is undefined" {file: "jar:file:///C:/Program%20Files/Nightly/omni.ja!/components/nsSearchService.js" line: 2080}]' when calling method: [nsIBrowserSearchService::currentEngine]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: resource://gre/modules/PlacesSearchAutocompleteProvider.jsm :: _refresh :: line 76"  data: yes]
    at _refresh(resource://gre/modules/PlacesSearchAutocompleteProvider.jsm:76:9)
    at initialize/</<(resource://gre/modules/PlacesSearchAutocompleteProvider.jsm:46:11)
    at onSuccess(jar:file:///C:/Program%20Files/Nightly/omni.ja!/components/nsSearchService.js:3730:13)

It looks like the first _metaData exception is from:

  getAttr(name) {
    return this._metaData[name] || undefined;
  },

Looks like we try to have this._metaData as an empty object, but from this it doesn't look like we always succeed.
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
I'm wondering if _metaData should just be {} here:

https://searchfox.org/mozilla-central/source/toolkit/components/search/nsSearchService.js#1278

instead of null?
Flags: needinfo?(florian)
(In reply to Mike Kaply [:mkaply] from comment #1)
> I'm wondering if _metaData should just be {} here:
> 
> https://searchfox.org/mozilla-central/source/toolkit/components/search/
> nsSearchService.js#1278
> 
> instead of null?

No, we shouldn't share an object in the prototype. It's correctly initialized to {} at https://searchfox.org/mozilla-central/rev/78dbe34925f04975f16cb9a5d4938be714d41897/toolkit/components/search/nsSearchService.js#1208

Also, the error says the value is 'undefined', not 'null'.

From looking at the code, the only place I see that could make this._metaData undefined on an engine object is: https://searchfox.org/mozilla-central/rev/78dbe34925f04975f16cb9a5d4938be714d41897/toolkit/components/search/nsSearchService.js#3194
We could add a null check before the assignment or add '|| {}' at the end of the line if we want to silence the error message.

But I wonder how users get into this situation, because our code should never write a cache file causing this error... Is it possible that users seeing this have third party software tinkering with our search file in the profile?
Flags: needinfo?(florian)
> We could add a null check before the assignment or add '|| {}' at the end of the line if we want to silence the error message.

We seem to do that in every other case involving metadata.
Priority: -- → P1
See Also: → 1466134
> But I wonder how users get into this situation, because our code should never write a cache file causing this error... Is it possible that users seeing this have third party software tinkering with our search file in the profile?

The answer to this question seems to be in bug 1466134.

A third party is writing to our search.json file and not putting in metadata.
Comment on attachment 8982673 [details]
Bug 1455261 - Check for null metadata to workaround broken search.json.

https://reviewboard.mozilla.org/r/248626/#review254880
Attachment #8982673 - Flags: review?(adw) → review+
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/9a5f5fa5cfa0
Check for null metadata to workaround broken search.json. r=adw
Drive by comments:
- I would prefer a null check done once during startup (as suggested in comment 2) instead of a null check done every time we read an attribute
- It would be nice to include a comment above this nullcheck to explain why it's needed, as that's not obvious when reading only the code.
> - I would prefer a null check done once during startup (as suggested in comment 2) instead of a null check done every time we read an attribute

If we do that, won't it end up fixing the broken metadata when the cache is written to disk? 

I'm wondering if we want to retain the broken metadata so we can at least tell it has been manipulated?
(In reply to Mike Kaply [:mkaply] from comment #9)
> > - I would prefer a null check done once during startup (as suggested in comment 2) instead of a null check done every time we read an attribute
> 
> If we do that, won't it end up fixing the broken metadata when the cache is
> written to disk? 

Yes, it would.

> I'm wondering if we want to retain the broken metadata so we can at least
> tell it has been manipulated?

I was wondering if it was the reason for doing the patch this way. I don't think the missing metadata object is a really interesting thing to preserve. As you noted in the other bug, the resource: iconURI for a supposedly used-installed plugin is more revealing. Doesn't matter much anyway.
Duplicate of this bug: 1466134
https://hg.mozilla.org/mozilla-central/rev/9a5f5fa5cfa0
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Opened bug 1466863 for the cleanup.
Should we uplift a fix for this (with or without the bug 1466863 cleanups included) into 61? We know it affects real users in the wild and prevents them from searching, and bug 1466134 proved it's due to hijacking (so it's likely going to affect an increasing number of users).
Flags: needinfo?(mozilla)
> Should we uplift a fix for this (with or without the bug 1466863 cleanups included) into 61?

I think that we should. Hadn't had a chance to response to RyanVM's wontfix.
Flags: needinfo?(mozilla)
Is this something we should be worried about on ESR60 also?
(In reply to Ryan VanderMeulen [:RyanVM] from comment #16)
> Is this something we should be worried about on ESR60 also?

Might be worth it since it's such a simple fix. I'll get the fix Florian requested done and we can go from there.
Flags: qe-verify+
I have the file for testing (It was sent separately).

QE can ping when they need it. It's pretty easy to test.
Hello guys,

I have followed the steps provided in the email forwarded from Mike and the search indeed works on the latest Nightly 62.0a1 (2018-06-13) with the same profile used in the affected FF version.

Confirming this issue as Verified-Fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Please request Beta/ESR60 approval on this when you get a chance.
Flags: needinfo?(mozilla)
Comment on attachment 8982673 [details]
Bug 1455261 - Check for null metadata to workaround broken search.json.

See https://bugzilla.mozilla.org/show_bug.cgi?id=1466863#c6 for details.

This bug needs to land first to avoid conflicts.
Flags: needinfo?(mozilla)
Attachment #8982673 - Flags: approval-mozilla-esr60?
Attachment #8982673 - Flags: approval-mozilla-beta?
Comment on attachment 8982673 [details]
Bug 1455261 - Check for null metadata to workaround broken search.json.

Needed for the uplift of bug 1466863. Verified by QA on Nightly. Approved for 61.0b14 and ESR 60.1.
Attachment #8982673 - Flags: approval-mozilla-esr60?
Attachment #8982673 - Flags: approval-mozilla-esr60+
Attachment #8982673 - Flags: approval-mozilla-beta?
Attachment #8982673 - Flags: approval-mozilla-beta+
Hello guys,

I have followed the steps provided in the email forwarded from Mike and the search indeed works on the latest Beta 61.0b14. 
Confirming this issue as fixed-verified for this build as well.
Hello guys,

I return after verifying with the steps provided in the email forwarded from Mike the 60.1.0esr build2. 
Confirming this issue as verified-fixed for this build as well.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.