Closed Bug 1466863 Opened 2 years ago Closed 2 years ago

Set metadata to {} instead of checking every time

Categories

(Firefox :: Search, enhancement, P1)

enhancement

Tracking

()

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

People

(Reporter: mkaply, Assigned: mkaply)

Details

Attachments

(1 file)

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.
Priority: -- → P1
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').
Attachment #8983954 - Flags: review?(florian) → review+
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/5404016c6d16
Just use empty metadata if invalid. r=florian
https://hg.mozilla.org/mozilla-central/rev/5404016c6d16
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Flags: qe-verify+
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).
Attachment #8983954 - Flags: approval-mozilla-esr60?
Attachment #8983954 - Flags: approval-mozilla-beta?
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.
Attachment #8983954 - Flags: approval-mozilla-esr60?
Attachment #8983954 - Flags: approval-mozilla-esr60+
Attachment #8983954 - Flags: approval-mozilla-beta?
Attachment #8983954 - Flags: approval-mozilla-beta+
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.
Status: RESOLVED → VERIFIED
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?
Flags: needinfo?(mozilla)
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.
Flags: needinfo?(mozilla)
Yeah. Sorry Mike. I downloaded the wrong esr. I retested on on 60.1.0esr build2 and indeed the issue is fixed.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.