1466863 - Set metadata to {} instead of checking every time

Status: VERIFIED FIXED

(Firefox :: Search, enhancement, P1)

Description

[Mike Kaply [:mkaply]]

Per Florian's comment in bug 1455261, we shouldn't check the value on every getAttr, we should just store the null metadata now that we know what the problem is.

Comment 1

[Mike Kaply [:mkaply]]

Attachment: Bug 1466863 - Just use empty metadata if invalid.

Review commit: https://reviewboard.mozilla.org/r/249800/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/249800/

Comment 2

[Florian Quèze [:florian]]

Comment on attachment 8983954 [details]
Bug 1466863 - Just use empty metadata if invalid.

https://reviewboard.mozilla.org/r/249800/#review256158

Looks good to me with the '|| undefined' re-added.

I think we'll want QA to verify this fix using the file provided in bug 1466134.

::: toolkit/components/search/nsSearchService.js:2046
(Diff revision 1)
>    setAttr(name, val) {
>      this._metaData[name] = val;
>    },
>  
>    getAttr(name) {
> -    return (this._metaData && this._metaData[name]) || undefined;
> +    return this._metaData[name];

The '|| undefined' was already there before your previous patch, I don't think we want to remove it. Without it I think we would cause JS strict warnings when accessing optional attributes (eg 'alias').

Comment 3

[Mike Kaply [:mkaply]]

Comment on attachment 8983954 [details]
Bug 1466863 - Just use empty metadata if invalid.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249800/diff/1-2/

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5404016c6d16
Just use empty metadata if invalid. r=florian

Comment 5

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/5404016c6d16

Comment 6

[Mike Kaply [:mkaply]]

Comment on attachment 8983954 [details]
Bug 1466863 - Just use empty metadata if invalid.

Approval Request Comment
[Feature/Bug causing the regression]: None
[User impact if declined]: Some users who get engines added by third party can't use their browser.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]:  No (already done)
[List of other uplifts needed for the feature/fix]: bug 1455261
[Is the change risky?]: No
[Why is the change risky/not risky?]: Just adds a null check.
[String changes made/needed]: None

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Some users can't use URL bar/search if they have hijacked.
Fix Landed on Version: 62
Risk to taking this patch (and alternatives if risky):  Very low
String or UUID changes made by this patch:  None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.


The reason Bug 1455261 is needed is to avoid conflicts. This patch is an improvement on the existing fix in 1455261 (which was tested and verified by QE).

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8983954 [details]
Bug 1466863 - Just use empty metadata if invalid.

Fixes broken search when some third parties add search engines. Approved for 61.0b14 and ESR 60.1.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/70c46487e513

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/9b9625e75c2f

Comment 10

[Catalin Sasca, Desktop Test Engineering [:csasca]]

The issue is verified fixed using the patch and STR provided in the e-mail forwarded from Mike. Tests were performed under latest Nightly 62.0a1 (2018-06-14) and Firefox 61.0b14.

Comment 11

[Catalin Sasca, Desktop Test Engineering [:csasca]]

Hi Mike. I verified the issue on 60.0.2esr - build 2, using the patch and STR provided in the e-mail and the search is not working. The test was performed under Windows 10 (x64). Do you have any idea about this?

Comment 12

[Mike Kaply [:mkaply]]

The correct ESR to test this on is 60.1.0

http://archive.mozilla.org/pub/firefox/candidates/60.1.0esr-candidates/build2/

60.0.2 was the security patch on the previous ESR.

Comment 13

[Catalin Sasca, Desktop Test Engineering [:csasca]]

Yeah. Sorry Mike. I downloaded the wrong esr. I retested on on 60.1.0esr build2 and indeed the issue is fixed.