1276025 - inIncognitoContext is always false in pageAction/browserAction popups

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P4)

Description

[blake.a.griffith]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160516004009

Steps to reproduce:

This is related to unifying the PrivacyBadger extension. If you have any suggestions on how to work around this, the work is here. https://github.com/EFForg/privacybadgerchrome/pull/845/

Steps to reproduce:
* open an private session 
* go to about debugging in the private session
* click 'debug' on my extension
* type `browser.extension.inIncognitoContext`, false is returned


Actual results:

got "false"


Expected results:

I should get "true"

Comment 1

[Kris Maglione [:kmag]]

That property is always true for background pages. It's only ever false in popups, tab pages, and content scripts.

Comment 2

[blake.a.griffith]

Kris, if I open the background page of this extension in chrome, and do `chrome.extension.inIncognitoContex` I get `false`. You are saying this should be `true`?

Comment 3

[Kris Maglione [:kmag]]

No, sorry, I meant it is always false for background pages.

Comment 4

[blake.a.griffith]

How should I inspect this property in the context that it is in a private session? I'm a noob.

I think something must be wrong because the same code works in chrome, but not firefox.

Comment 5

[Kris Maglione [:kmag]]

We don't support anything like Chrome's split incognito mode. Popups and tabs in private browsing windows are in private browsing contexts. The background page never is. All of the contexts are in the same session.

Comment 6

[blake.a.griffith]

Shouldn't not supporting the manifest.json incognito key be mentioned in the API reference? Is there a way to disallow a webextension in private browsing mode? Like `"not_allowed"` in chrome (I tried using this and it didn't work).

Not supporting this could cause big security problems for a lot of extensions that track user history and be an immediate non-starter. Consider an extension that tracks my time visiting websites, I wouldn't want sites visited in private browsing mode showing up there.

Comment 7

[Andy McKay]

Implementing something like "incognito": "not_allowed" would work for me. I believe that landed in 47. Morph this bug into that?

https://developer.chrome.com/extensions/manifest/incognito

Comment 8

[Andy McKay]

Although after typing that it did occur to me that implementing that means solving the original problem of figuring out when you are in private browsing mode or not, so probably hasn't made the bug any easier.

Comment 9

[blake.a.griffith]

Andy, I'm not sure I understand. "incognito": "not_allowed" landed in Chrome 47?

I'm also not sure what you mean, that I would need to figure out when I'm in private browsing mode.
My extension would not have to figure out when it is in private browsing mode if "not_allowed" is implemented, because it would never be in private browsing mode.

Implementing this would be good for now. That is what chrome started with. This mode really should be the default until chrome-like split and spanning are implemented. Is there a plan to implement these later?

Comment 10

[Andy McKay]

Yes, Chrome 47. Your extension wouldn't have to figure out when its private browsing mode, but the rest of the WebExtension framework in Firefox would have to. Kris already pointed out in comment 5 that we don't have the split mode at all. I'm not sure if implementing "not_allowed", is particularly easier than a split mode, maybe it is, maybe not.

This isn't on our roadmap for implementation yet unless a contributor decided to take it on.

Comment 11

[blake.a.griffith]

Is current plan is to allow all extensions to work in private browsing mode by default?

Comment 12

[blake.a.griffith]

Kris, should `inIncognitoContext` be true in a popup (browser action script) in private browsing mode? It is false for me. The only way I'm able to get `inIncognitoContext` to be true is in content scripts. This is a bug right?

Comment 13

[Kris Maglione [:kmag]]

Hm. You're right, this is broken for popup documents, because we currently create their contexts differently from most contexts.

It does work in extension pages loaded into tabs, though.

Comment 14

[blake.a.griffith]

My extension currently has no page actions, so for me it is easier to include it in a content script.

Comment 15

[Kris Maglione [:kmag]]

Attachment: Bug 1276025: Stop using injectInDocShell to tag docShells with types.

Review commit: https://reviewboard.mozilla.org/r/57496/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57496/

Comment 16

[Kris Maglione [:kmag]]

Comment on attachment 8759498 [details]
Bug 1276025: Stop using injectInDocShell to tag docShells with types.

On second thought, some of this is pretty arcane. It should probably be looked at by someone with a bit more docshell experience.

Bill, can you please look at the docshell/browser portions of this?

Thanks

Comment 17

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/57496/#review54420

One nitpick: there are still some leftover bits in Extension.jsm that appear to be unused (initializing docShells and cleaning it up).
Other than that, I don't see any glaring problems but I don't understand the connection between the changes here and the bug that this addresses, can you give a little background and then I can look again with that in mind?

::: toolkit/components/extensions/Extension.jsm:634
(Diff revision 1)
>        return;
>      }
>  
> +
>      let docShell = contentWindow.QueryInterface(Ci.nsIInterfaceRequestor)
> -                                .getInterface(Ci.nsIWebNavigation)
> +                                .getInterface(Ci.nsIDocShell);

I don't really understand this change.  Was the previous chain of QI, etc just unnecessary or was it related to other code that you removed?

::: toolkit/components/extensions/Extension.jsm:640
(Diff revision 1)
> -    let extension = this.extensionMap.get(id);
> -    let uri = contentWindow.document.documentURIObject;
> +    let parentDocument = docShell.parent.QueryInterface(Ci.nsIDocShell)
> +                                 .contentViewer.DOMDocument;
> -    let incognito = PrivateBrowsingUtils.isContentWindowPrivate(contentWindow);
>  
>      let browser = docShell.chromeEventHandler;
> +    if (contentWindow.frameElement &&

I'm not following this part, perhaps a comment to explain it?

Comment 18

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/57496/#review54420

> I don't really understand this change.  Was the previous chain of QI, etc just unnecessary or was it related to other code that you removed?

The previous version gave us the top-level content docshell, rather than the docshell for the frame we're actually injecting into. That was sort of necessary when we had injectInDocShell, to get the view type for subframes right, but it comes with a whole set of problems.

> I'm not following this part, perhaps a comment to explain it?

Basically, if this window is a frame that's loaded into a chrome-privileged window (namely about:addons), we need to look at the frame element rather than the <browser> that's holding the tab content.

Comment 19

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/57496/#review54420

This bug happens because we currently create page contexts for popups differently than we create ordinary page contexts, and using different logic. With these changes, we create all contexts in the same way, so the `incognito` property is always correct.

It fixes some other issues as well, like the same context being reused for multiple extension pages loaded into the same popup, or into subframes of the same popup, but the `inIncognitoContext` issue is the one I'm trying to address at the moment.

Comment 20

[blake.a.griffith]

In "spanning" mode in chrome, `inIncognitoContext` is false in the popup. So this probably is not bug, sorry for the run-around and thanks for the help.

Comment 21

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8759498 [details]
Bug 1276025: Stop using injectInDocShell to tag docShells with types.

https://reviewboard.mozilla.org/r/57496/#review54876

Look good. Thanks.

::: browser/components/extensions/ext-tabs.js:60
(Diff revision 1)
> -    let tab = null;
> -    if (params.type == "tab") {
> +    let tab = parentWindow.gBrowser.getTabForBrowser(browser);
> +    if (tab) {

What's the purpose of this change? Won't getTabForBrowser be non-null if and only if params.type == "tab"? Although I guess that wasn't true before you fixed the about:addons issue?

::: toolkit/components/extensions/Extension.jsm
(Diff revision 1)
>    getExtension(extensionId) {
>      return this.extensionMap.get(extensionId);
>    },
>  
> -  injectInDocShell(docShell, extension, context) {
> -    this.docShells.set(docShell, {extension, context});

If you're removing this, you should remove GlobalManager.docShells as well as the use in uninit().

::: toolkit/components/extensions/Extension.jsm:642
(Diff revision 1)
> -    let incognito = PrivateBrowsingUtils.isContentWindowPrivate(contentWindow);
>  
>      let browser = docShell.chromeEventHandler;
> +    if (contentWindow.frameElement &&
> +        Services.scriptSecurityManager.isSystemPrincipal(parentDocument.nodePrincipal)) {
> +      browser = contentWindow.frameElement;

This line definitely needs a comment about what it's for. It might also be clearer if you instead check whether the document URL is about:addons. I'd rather make this check as tightly scoped as possible so it doesn't start applying to something else by accident.

::: toolkit/components/extensions/Extension.jsm:646
(Diff revision 1)
> +        Services.scriptSecurityManager.isSystemPrincipal(parentDocument.nodePrincipal)) {
> +      browser = contentWindow.frameElement;
> +    }
>  
>      let type = "tab";
>      if (browser instanceof Ci.nsIDOMElement) {

Just curious: can chromeEventHandler ever not be a DOM element. From my reading of the code, it comes from a frameloader's mOwnerContent, which I would expect to be an element.

Comment 22

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/57496/#review54876

> What's the purpose of this change? Won't getTabForBrowser be non-null if and only if params.type == "tab"? Although I guess that wasn't true before you fixed the about:addons issue?

For options pages, the type is "popup", but it's still the child of a tab, and `tabs.getCurrentTab()` needs to return that tab. This change makes that work.

> This line definitely needs a comment about what it's for. It might also be clearer if you instead check whether the document URL is about:addons. I'd rather make this check as tightly scoped as possible so it doesn't start applying to something else by accident.

Yeah, I guess explicitly checking for about:addons makes sense.

> Just curious: can chromeEventHandler ever not be a DOM element. From my reading of the code, it comes from a frameloader's mOwnerContent, which I would expect to be an element.

It can be null for top-level docshells, but it can't be any other type. I think there was a reason I originally checked `instanceof` rather than null, but checking for null probably makes more sense now.

Comment 23

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/99b213f494e1a9e20629e936681faedec16d22a3
Bug 1276025: Stop using injectInDocShell to tag docShells with types. r=billm

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/b93f2b53838a for xpcshell bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=9842073&repo=fx-team

Comment 25

[Kris Maglione [:kmag]]

Well, this will teach me not to think that a patch certainly won't affect any xpcshell tests...

Comment 26

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/a7b31be0a19ae5413db3e10ad4e5251cac84b08d
Bug 1276025: Stop using injectInDocShell to tag docShells with types. r=billm

Comment 27

[Carsten Book [:Tomcat]]

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