1552559 - Some One-Click Search Engines are still constantly readded after each Mozilla Firefox update after landing patches from bug #1496075

Status: VERIFIED FIXED

(Firefox :: Search, defect, P1)

Description

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

+++ This bug was initially created as a follow-up of Bug #1544273 +++

Looks like it wasn't fixed completely, as it's still occurs using even latest Mozilla Firefox Nightly 68.0a1 (2019-05-17) (64-bit) [rev: 839cdad764d741ab4438b6feabeec749a22b34d5].

STR:

1. Go to Options ( about:preferences )
2. Select "Search" tab ( about:preferences#search )
3. Remove all One-Click Search Engines leaving only "Google"
4. Wait for daily Mozilla Firefox Nightly update
   and after update doing STR #1 & #2, you can see that:

• Amazon.com
• eBay
  are readded to One-Click Search Engines

Regression caused by:
bug #1496075

@ Dale Harvey (:daleharvey) - I attached Browser Console log with "browser.search.log" boolean type preference set to "true" value in about:config.

Comment 1

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: Browser Console.txt

Comment 3

[Dale Harvey (:daleharvey)]

Argh ok will take a look again, cheers for the report + log

Comment 4

[Pulse]

Unfortunately, this is still happening. In my case Amazon, ebay and Yandex are re-added after each update.

Comment 6

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Comment 7

[Dale Harvey (:daleharvey)]

ok so the things we pull out from the extension manifest and undefined here which is definitely not what we expect, Virtual thanks for all the help here, could you possibly get the log again with the pref |extensions.logging.enabled| set to true, there is something going wrong with extensions it looks like

Comment 8

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Sure thing! I will post it when next update will happen. Should I also set "browser.search.log" to "true" in about:config, or it isn't needed this time to prevent too much "spam"?

Comment 9

[Dale Harvey (:daleharvey)]

Yeh both logs would be very useful, thanks

Comment 10

[Dale Harvey (:daleharvey)]

Attachment: Bug 1552559 - Fix readyPromise usage to ensure extension has started.

Comment 12

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: Browser Console.txt

On browser restart after update, bug doesn't occur, in Options only Google is visible, everything looks good.

Comment 13

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: Browser Console - after second browser restart.txt

But after second browser restart, eBay search engine was readded to One-Click Search Engines under Search tab in Options.

Comment 14

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: Browser Console.txt

Today, after update restart, bug occurred on first try, no second restart was needed.

Comment 15

[Dale Harvey (:daleharvey)]

Attachment: Bug 1552559 - Ensure built in engines are available on startup.

Comment 16

[Dale Harvey (:daleharvey)]

Attachment: Bug 1552559 - Ensure built in engines are available on startup.

Comment 17

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1552559 part 2: refactor extension install in searchservice to use promises

Comment 18

[Dale Harvey (:daleharvey)]

Cheers Virtual_ManPL, found the issue and working on the fix (clearing needinfo)

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5e8198871a11
Ensure built in engines are available on startup. r=mikedeboer

Comment 20

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/5e8198871a11

Comment 21

[Julien Cristau [:jcristau] (back April 22)]

Please request beta uplift when you feel comfortable doing so.

Comment 22

[:gerard-majax]

I (and others) have been bitten by a nasty bug where about:newtab and the address bar are totally broken, since today. Running mozregression lead me to this bug:

5:56.88 INFO: Narrowed inbound regression window from [f31bc32e, 5e819887] (3 builds) to [cd4d4f19, 5e819887] (2 builds) (~1 steps left)
5:56.88 INFO: No more inbound revisions, bisection finished.
5:56.88 INFO: Last good revision: cd4d4f19250633c7d8fba388a555ca4faf93ded0
5:56.88 INFO: First bad revision: 5e8198871a11f420bc07ba8d33126713a4332ff7
5:56.88 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=cd4d4f19250633c7d8fba388a555ca4faf93ded0&tochange=5e8198871a11f420bc07ba8d33126713a4332ff7

Comment 23

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

I'm confirming that bug is fixed, starting in Mozilla Firefox Nightly 69.0a1 (2019-06-03), so I'm marking this bug as VERIFIED.
Thank you very much! \o/

Comment 24

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Dale, please request uplift for Beta68 if you think it's worth shipping this fix in 68.

Comment 25

[Pulse]

Hate to say this but I just had ebay reappear in my search engine list. Unfortunately, I didn't have logging enabled. I'm on Nightly and I'll re-enable search logging and report back if it happens again.

Comment 26

[Dale Harvey (:daleharvey)]

ok cheers, and yeh if you do see it happen again please file a new bug, cheers

Comment 27

[Dale Harvey (:daleharvey)]

Comment on attachment 9067046 [details]
Bug 1552559 - Ensure built in engines are available on startup.

Beta/Release Uplift Approval Request

• User impact if declined: Reloading hidden search engines
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: 1. Start firefox with new profile

2. Visit about:preferences#search
3. Delete various engines
4. Restart with RELOAD_ENGINES=1 flag
5. Engines should not reappear

• List of other uplifts needed: https://bugzilla.mozilla.org/show_bug.cgi?id=1556424
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): Refactor which made the code much simpler to reason, is covered pretty well by existing tests but unable to create a new test that hit this specific startup race
• String changes made/needed:

Comment 28

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 9067046 [details]
Bug 1552559 - Ensure built in engines are available on startup.

regression fix for 68.0b10

Comment 29

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/releases/mozilla-beta/rev/52554b40948e

Comment 30

[Adrian Florinescu [:aflorinescu]]

In order to verify the fix, first we have tried on our side to reproduce the issue with affected versions(nightly from 06.01/06.02/05.17/05.18 and 68beta8). We used as environments W10/W7 both 32b and 64b with no success. (simulated update with pave-over and used the env. variable as specified in comment 27).

In this situation, we can only validate that the issue doesn't reproduce after the fix gets uplifted in beta10. Dale, do you consider verifying that the issue doesn't reproduce after the uplift is enough?

Virtual_ManPL, perhaps you'd like to extend a helping hand reproducing the issue on affected 68 beta and then verifying it after the fix? Or maybe suggest what we are missing into reproducing successfully the issue.

Comment 31

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

@ Adrian Florinescu [:adrian_sv] - I can confirm that I can't reproduce this bug anymore, when I got today update from Mozilla Firefox 68.0b9 to 68.0b10.
About STR, you probably need to have some "borked" profile like me, but I'm not specialist in this matter, so maybe Dale Harvey (:daleharvey) will provide you technical information about this how to reproduce it.

Comment 32

[Adrian Florinescu [:aflorinescu]]

Thanks for verification!
Removing the Ni from Dale, since it this issue is already verified thanks to Virtual_ManPL assistance.