1352102 - Allow extensions to read the value of the browser.privatebrowsing.autostart preference

Status: RESOLVED WONTFIX

(WebExtensions :: General, enhancement, P3)

Description

[Jonathan Kingston [:jkt] he/him]

Bug 1312802 adds the privacy API however I can't get access to "browser.privatebrowsing.autostart" which essentially makes all tab.incognito=true

In nightly containers we permit the use of this mode to have containers but we disallow them for normal incognito windows to simplify the interface. We currently can't get this working in the same way for the containers test pilot using web extensions.

Comment 1

[Bob Silverberg [:bsilverberg]]

I'm not really understanding what you're asking for. Is this a request to add a new setting to the privacy API which would allow a WebExtension to control the value of the browser.privatebrowsing.autostart preference, or is there more to it than that?

Comment 2

[Jonathan Kingston [:jkt] he/him]

No I don't think we can do that without restart.
I am requesting for a way to detect what the state of this pref is. That way we know if the users tabs are always incognito I still can provide containers functionality.

Comment 3

[Bob Silverberg [:bsilverberg]]

The privacy API includes methods to get, set and clear a setting, so if we need a way for an extension to know the value of this pref, the privacy API probably makes sense for that. It sounds like you're saying we would want this to be read-only though - is that the case? I.e., an extension can call get, but set and clear would either generate a warning/exception, or simply be ignored. I'm not sure if that fits in well with what we want for the privacy API, but it's something to consider.

Comment 4

[Jonathan Kingston [:jkt] he/him]

I'm not picky where it gets placed, that was just was a suggestion. Perhaps it would make sense on a tab/window object instead as this is likely the only place a developer would need to interact with it.

We could however do tests and see if it is restart proof, currently setting the "never remember history" in the privacy panel requires a browser restart. From memory I think it is because of how history/awesomebar handles itself requires the restart.

I'm also not too upset by the API just throwing exceptions to say that setting/clearing isn't supported yet.

Comment 5

[Caitlin Neiman [:caitmuenster]]

This has been added to the agenda for the April 25 WebExtensions Triage meeting. 

Jonathan, would you be able to join us? 

Call-in info and time: https://wiki.mozilla.org/Add-ons/Contribute/Triage#Details_.26_How_to_Join

Agenda: https://docs.google.com/document/d/1WScwIH2D5tiL7vT4_lC44oFigR53xatWsUEwyjWvs7k/edit#

Comment 6

[Jonathan Kingston [:jkt] he/him]

Hey Caitlin, I should be able to make the meeting, thanks for adding it to the agenda.

Comment 7

[Bob Silverberg [:bsilverberg]]

We discussed this in the WebExtensions Triage meeting and it was approved. The scope of this is just to provide extensions with the ability to read the value of the browser.privatebrowsing.autostart preference. We could implement this as a new setting for the privacy API, or we could just make it a property of each tab and/or window. If the former, it would be a read-only setting, and we would throw if anyone tried to set the setting.

When this gets picked up we can finalize the implementation.

Comment 8

[Luca Greco [:rpl] [:luca] [:lgreco]]

Hey Bob,
if I'm reading Comment 0 correctly, we probably want to let a webextension to know about the value of PrivateBrowsingUtils.isInTemporaryAutoStartMode (http://searchfox.org/mozilla-central/search?q=PrivateBrowsingUtils.isInTemporaryAutoStartMode&path=), because the extension described in the summary will need to know that the browser is running in the "always private mode" (vs. "knowing if the preference has been set", which can be set to true while the browser have not been restarted yet).

Comment 9

[Bob Silverberg [:bsilverberg]]

(In reply to Luca Greco [:rpl] from comment #8)
> Hey Bob,
> if I'm reading Comment 0 correctly, we probably want to let a webextension
> to know about the value of PrivateBrowsingUtils.isInTemporaryAutoStartMode
> (http://searchfox.org/mozilla-central/search?q=PrivateBrowsingUtils.
> isInTemporaryAutoStartMode&path=), because the extension described in the
> summary will need to know that the browser is running in the "always private
> mode" (vs. "knowing if the preference has been set", which can be set to
> true while the browser have not been restarted yet).

That sounds accurate, Luca. Jonathan, can you confirm that that is in fact what you want?

Comment 10

[Jonathan Kingston [:jkt] he/him]

We actually have used the permanentPrivateBrowsing mode in the past which is as I understand it temporary || the pref.

https://reviewboard.mozilla.org/r/96122/diff/1#index_header

This is seemingly more consistent with the other code we have in core.

Comment 11

[Jorge Villalobos [:jorgev] (he/him)]

If this is your first contribution, please refer to https://wiki.mozilla.org/Add-ons/Contribute/Code on how to get started. 

Mentor: :rpl

Comment 12

[Tim B [:tb120]]

I'd like to work on this bug and get some experience with the APIs. It will only be my second bug fix so I would appreciate any input. Thanks.

Comment 13

[Tim B [:tb120]]

I was hoping to get some input on what I've done, which is based on the comments above and the existing code. I thought that it might be easier to provide some suggestion based on my understanding, correct or incorrect. I haven't pushed anything because I'm trying to learn how the testing structure is setup.

In `ext-privacy.js`:

1. I added a new setting called "privacy.incognito.alwaysPrivateBrowsingEnabled" and registered it with the `ExtensionPreferencesManager` although I don't have any `prefNames` and the callback returns an empty object.

2. The callback for the new setting to `getPrivacyAPI` returns the value of `!PrivateBrowserUtils.isInTemporaryAutoStartMode()`.

3. I added an array of read only settings that are checked in the `set` and `clear` functions. If the `name` of the setting is in that list, it throws an `ExtensionError`.

Thanks for the input.

Comment 14

[Andrew Swan [:aswan]]

Tim, if you have a specific question and you're waiting for an answer from somebody, please check the "Need more information from" button at the bottom of this page when submitting a comment.  If you're not sure who to ask, the mentor for the bug (Luca Greco :rpl) is a good default choice.

For this particular bug, there is no need to use the ExtensionPreferencesManager, that's for code that changes the value of browser preferences, which is not the case here.  The goal here is just to expose a BrowserSetting interface:
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/types/BrowserSetting

This setting will be read-only to extensions which means all you really need to implement is get().  set() and clear() can have stub implementations that throw an exception with an appropriate error message.

Comment 15

[Tim B [:tb120]]

Andrew, thanks for the input. I appreciate the help. Can you clarify something for me though? Given the above, will this be added to browserSettings API (ext-browserSettings.js) instead of the privacy API (ext-privacy.js)? Thanks for the help.

Comment 16

[Luca Greco [:rpl] [:luca] [:lgreco]]

The privacy and browserSettings API are pretty similar to each other (also from an implementation point of view, BrowseSetting is the interface used in both these APIs), the main difference between them is that the privacy API is strictly related to the privacy-related settings, and so in my opinion this looks like a settings that should be exposed as part of the privacy API (e.g. as a new privacy.websites.permanentPrivateBrowsingEnabled read-only setting).

Comment 17

[Tim B [:tb120]]

I added a new setting called "privacy.incognito.alwaysPrivateBrowsingEnabled". Was that the correct thing to do? I'm not attached to the name(s) so I'm more than willing to change them. Also, are xpcshell the only required tests or do I need some type of mochi test as well? Thanks for the help.

Comment 18

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Tim B [:tb120] from comment #17)
> I added a new setting called
> "privacy.incognito.alwaysPrivateBrowsingEnabled". Was that the correct thing
> to do? I'm not attached to the name(s) so I'm more than willing to change
> them. 

Personally I would vote for "privacy.websites.permanentPrivateBrowsingEnabled" ;-)

> Also, are xpcshell the only required tests or do I need some type of
> mochi test as well? Thanks for the help.

At a first glance I think that it should be doable to test this from an xpcshell test, and so I would aim for an xpcshell test first (as they run way faster then mochitests).
We can re-evaluate this plan if we find issues on testing it from an xpcshell test or if we are not happy enough with the coverage that we achieved with it.

Comment 19

[Caitlin Neiman [:caitmuenster]]

Hey Tim! How's it going with this bug?

Comment 20

[Tim B [:tb120]]

It was going well and then I lost track of time. I was working on the testing portion before I got distracted elsewhere. I hope to push soon.

Comment 21

[Caitlin Neiman [:caitmuenster]]

Hey Tim! Just checking in again. :) If you've got something you'd like to have reviewed, you can go ahead and push it to Phabricator.

Comment 22

[Caitlin Neiman [:caitmuenster]]

Hey Tim, I am going to open this bug up for other contributors to work on. If you'd like to, you can still submit a patch.

Comment 23

[Rizwan Syed]

Hey Caitlin. I have patched a few bugs and am looking to move up from the 'good-first-bug' bugs. This bug looks interesting - although I don't have the faintest clue about how to solve it. The comments and descriptions above use a lot of jargon that I don't understand. Is this bug mentored to a level that is suitable for me?

Comment 24

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Rizwan Syed from comment #23)

Hey Caitlin. I have patched a few bugs and am looking to move up from the 'good-first-bug' bugs. This bug looks interesting - although I don't have the faintest clue about how to solve it. The comments and descriptions above use a lot of jargon that I don't understand. Is this bug mentored to a level that is suitable for me?

Hi Rizwan,
all the jargon used in the previous comments should become much more clear once you start to look into the kind of changes needed to fix this issue.

A good starting point is often taking a look to the existing code, e.g. starting from files or components mentioned in the above comments.
You can quickly look into what ext-browserSettings.js and ext-privacy.js do by searching for them on searchfox.org (searchfox is very helpful to quickly explore the Firefox sources):

• https://searchfox.org/mozilla-central/source/toolkit/components/extensions/parent/ext-privacy.js
• https://searchfox.org/mozilla-central/source/toolkit/components/extensions/parent/ext-browserSettings.js

To see how most of the settings ext-privacy.js are being exposed, you can take a look to the helper getPrivacyAPI defined in ext-privacy.js itself, but for this setting we don't need set or clear (because the extensions shouldn't be able to control this preference) and so, as aswan mentioned in comment 14, this patch shouldn't need to use getPrivacyAPI or ExtensionPreferenceManager, but just the implementation for the single get() method (which should just retrieve the current value of the preference and return it to the caller).

Once you have started to give a try to what kind of change needed by this bug, you may also start to have additional (and more specific) questions about these or new details, and so let me know if you need help by adding a needinfo assigned to me.

Comment 25

[Shane Caraveo (:mixedpuppy)]

I'm not sure we should do this. The use case in comment 2 should be handled inside the API in question. It is the API implementation (ie. contextualIdentities) that should determine how it works in PPB, not the extension trying to assume it will work a certain way under different settings.

As well, I think an extension can already detect this by checking extension.inIncognitoContext in the background script, which would only ever be true in PPB (at least until/if we implement support for split mode).

Lets throw this back to mconca for now, and wait to do any work on it.

Comment 26

[Mike Conca [:mconca]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #25)

As well, I think an extension can already detect this by checking extension.inIncognitoContext in the background script, which would only ever be true in PPB (at least until/if we implement support for split mode).

Is this true? The extension.inIncognitoContext seems mostly (only?) useful for content scripts, and the value of that property in an extension background page isn't clearly defined when the extension is running as "incognito:spanning" (the default). This seems like a good MDN documentation opportunity.

Lets throw this back to mconca for now, and wait to do any work on it.

I'm also thinking we shouldn't do this. Adding an API to determine if Firefox is in permanent private browsing mode seems very limited, appealing only to extensions that want to do something different in perma-PBM versus when the user opens private windows individually. That's not really a practice I want to encourage, despite the fact that Mozilla's container extension seems to be doing this. I'd rather developers simply handle private and non-private windows appropriately within the extension.

Comment 27

[Shane Caraveo (:mixedpuppy)]

(In reply to Mike Conca [:mconca] from comment #26)

(In reply to Shane Caraveo (:mixedpuppy) from comment #25)

As well, I think an extension can already detect this by checking extension.inIncognitoContext in the background script, which would only ever be true in PPB (at least until/if we implement support for split mode).

Is this true? The extension.inIncognitoContext seems mostly (only?) useful for content scripts, and the value of that property in an extension background page isn't clearly defined when the extension is running as "incognito:spanning" (the default). This seems like a good MDN documentation opportunity.

We have a test that verifies it can be used in teh background page, one that explicitly tests it is true for PPB.

https://searchfox.org/mozilla-central/source/toolkit/components/extensions/test/xpcshell/test_ext_background_private_browsing.js

It would be false when not in PPB, spanning or not.

If we do split mode, if there is a reason an extension needs to know that it is running in PPB, then we would probably benefit from adding this. But I don't see a particular reason to implement split mode (doesn't mean there isn't one).

Comment 28

[Rizwan Syed]

Hey all, after seeing this discussion about doing this, I will leave it - until there is clear info on whether open source contributors should work on this bug.

Cheers

Comment 29

[Shane Caraveo (:mixedpuppy)]

Rizwan, thank you! Stepping in and offering to work on this caused us to rethink it.

For now I'm going to close this bug and wontfix.