further fixups for homepage/newtab

VERIFIED FIXED in Firefox 67

Status

defect
P2
normal
VERIFIED FIXED
3 months ago
a month ago

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

Tracking

(Blocks 1 bug)

unspecified
mozilla68
Dependency tree / graph

Firefox Tracking Flags

(firefox67 verified, firefox68 verified)

Details

(Whiteboard: [qa-triaged])

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

3 months ago

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.

(Assignee)

Updated

3 months ago
Flags: needinfo?(mconca)
Flags: needinfo?(ddurst)
(Assignee)

Updated

3 months ago
Priority: -- → P2

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

Flags: needinfo?(mconca)
Flags: needinfo?(ddurst)
(Assignee)

Comment 3

2 months ago

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

Flags: needinfo?(rob)
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(aswan)

Comment 4

2 months ago

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?

Flags: needinfo?(rob)
(Assignee)

Comment 5

2 months ago

(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

2 months ago

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.

(Assignee)

Comment 7

2 months ago

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.

(Assignee)

Updated

2 months ago
Assignee: nobody → mixedpuppy

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

Flags: needinfo?(aswan)
(Assignee)

Updated

2 months ago
See Also: → 1535069
(Assignee)

Updated

2 months ago
Type: enhancement → defect
Attachment #9048650 - Attachment is obsolete: true

Comment 13

a month ago
Pushed by scaraveo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e8634d10ef03
use prefs to support extension newtab and homepage on startup. r=kmag

Comment 14

a month ago
bugherder
Status: NEW → RESOLVED
Last Resolved: a month ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
(Assignee)

Updated

a month ago
Depends on: 1543538
No longer depends on: 1543538
Regressions: 1543538
(Assignee)

Comment 15

a month ago

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
Attachment #9055212 - Flags: approval-mozilla-beta?
(Assignee)

Updated

a month ago
Flags: qe-verify?

Let's verify on Nightly first.

Flags: qe-verify? → qe-verify+

Comment 17

a month ago
Posted image 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.

Updated

a month ago
Status: RESOLVED → VERIFIED

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.

Attachment #9055212 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Assignee)

Updated

a month ago
Flags: needinfo?(kmaglione+bmo)
Whiteboard: [qa-triaged]

Comment 20

a month ago
Posted image 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.

Updated

a month ago
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.