1532165 - further fixups for homepage/newtab

Status: VERIFIED FIXED

(WebExtensions :: General, defect, P2)

Description

[Shane Caraveo (:mixedpuppy)]

Bug 1532110 is fixing a problem loading homepage when in PPB. There is still an edge case that was meant to be fixed by using ExtensionsSettingsStore, however that is not yet initialized on startup, though it only affects PPB.

Proposed solutions that may work:

• sync init ExtensionSettingsStore when in PPB

• use a second pref to mark the homepage_override as controlled by an extension with private permission. This quickly gets complicated as it would need to be updated when the private permission changes.

• Never use homepage_override from any extension in PPB. We would still need a secondary pref to easily identify that it is extension controlled, however we would not need to know the extensions permissions.

Comment 1

[Mike Conca [:mconca]]

Let's pursue option 1 (sync init) and see what the performance hit is for startup in PPB mode.

Comment 2

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1532165 sync read extension permissions on startup on PPB

Comment 3

[Shane Caraveo (:mixedpuppy)]

I'm still less than enthused by this, even if it only effects PPB. Asking around for any better idea.

Comment 4

[Rob Wu [:robwu]]

Is it feasible to pretend that extensions without the private permission are disabled if perma-private browsing mode is enabled? E.g. by storing the state of the internal private browsing permission in startupData? And when an add-on is enabled while PPB is enabled, implicitly grant the internal private browsing permission to the extension?

Comment 5

[Shane Caraveo (:mixedpuppy)]

(In reply to Rob Wu [:robwu] from comment #4)

Is it feasible to pretend that extensions without the private permission are disabled if perma-private browsing mode is enabled? E.g. by storing the state of the internal private browsing permission in startupData?

I thought startupData was loaded async. The homepage/newtab getters are synchronous and called very early. We also decided against putting the permission in startupData very early (in lieu of changing it to use ExtensionPermissions).

And when an add-on is enabled while PPB is enabled, implicitly grant the internal private browsing permission to the extension?

IIUC you're suggesting the extension automatically gets private access. We removed that on purpose.

Comment 6

[Rob Wu [:robwu]]

The URL must be synchronously available, and the private browsing status is not synchronously available.
A way to resolve this (other than the many options that you already have) is to return a dummy URL that resolves when the extra extension data is ready. I.e. moz-extension-redirect://search-provider/ that may eventually resolve to moz-extension://UUID/, about:home or about:privatebrowsing depending on the permission.

Or without introducing a new scheme/URL: return the moz-extension:// URL unconditionally (by the homepage getter) and let the protocol handler (ExtensionProtocolHandler) redirect to about:privatebrowsing if the private browsing permission is not available AND the requested URL is the same as the one in the chrome_settings_overrides.homepage section of manifest.json.

Comment 7

[Shane Caraveo (:mixedpuppy)]

An extension can set the homepage to an http url, so relying on the extension protocol wont work in that case. We do not want to simply allow those urls to go through if the extension does not have private access.

Comment 8

[Andrew Swan [:aswan]]

As discussed on IRC, I think the current approach (synchronously reading settings in permanent private mode) is probably the best option.

Comment 9

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1532165 use prefs to support extension newtab and homepage on startup.

Comment 10

[Shane Caraveo (:mixedpuppy)]

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

Comment 11

[Shane Caraveo (:mixedpuppy)]

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

Comment 12

[Shane Caraveo (:mixedpuppy)]

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e8634d10ef03
use prefs to support extension newtab and homepage on startup. r=kmag

Comment 14

[Daniel Varga [:dvarga]]

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

Comment 15

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 9055212 [details]
Bug 1532165 use prefs to support extension newtab and homepage on startup.

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: Bug 1380809
• User impact if declined: An extension that does not have permission for private windows may still be able to set a homepage or newtab to an external url.

Also requires test fix from bug 1543538

• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: install an extension that sets homepage to an http url
  do not give private permission
  set permanent private browsing, restart
  you should not get the external url as a homepage
• List of other uplifts needed: Bug 1543538
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): It is a larger change than prefered for uplifting at this point, but this does prevent setting a homepage for private browsing when the extension does not have permission.
• String changes made/needed: none

Comment 16

[Pascal Chevrel:pascalc]

Let's verify on Nightly first.

Comment 17

[CosminB]

Attachment: Bug1532165.gif

This issue is verified as fixed on Firefox 68.0a1 (20190414214746) under Win 7 64-bit and Mac OS X 10.14.1.

Please see the attached video.

Comment 18

[Pascal Chevrel:pascalc]

Comment on attachment 9055212 [details]
Bug 1532165 use prefs to support extension newtab and homepage on startup.

Verified by QA on Nightly and has tests, approved for 67 beta 11. However, since this is a larger uplift than usual, if this causes new regressions in the rest of the beta cycle, we will back the patch out and let it ride the trains to 68. Thanks.

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/f5f1b91cae28

Comment 20

[CosminB]

Attachment: Bug1532165-Beta.gif

This issue is verified as fixed on Firefox 67.0b11 (20190415085659) under Win 7 64-bit and Mac OS X 10.14.1.

Please see the attached video.