Closed Bug 1276025 Opened 4 years ago Closed 3 years ago

inIncognitoContext is always false in pageAction/browserAction popups

Categories

(WebExtensions :: Untriaged, defect, P4)

48 Branch
defect

Tracking

(firefox50 fixed)

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: blake.a.griffith, Assigned: kmag)

References

Details

(Whiteboard: triaged)

Attachments

(1 file)

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"
That property is always true for background pages. It's only ever false in popups, tab pages, and content scripts.
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`?
No, sorry, I meant it is always false for background pages.
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.
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.
Priority: -- → P4
Whiteboard: triaged
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.
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
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.
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?
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.
Is current plan is to allow all extensions to work in private browsing mode by default?
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?
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: browser.extension.inIncongnitoContext broken, but is supposed to work → inIncognitoContext is always false in pageAction/browserAction popups
My extension currently has no page actions, so for me it is easier to include it in a content script.
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
Attachment #8759498 - Flags: review?(wmccloskey)
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?
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.
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.
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 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.
Attachment #8759498 - Flags: review?(wmccloskey) → review+
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.
Well, this will teach me not to think that a patch certainly won't affect any xpcshell tests...
Flags: needinfo?(kmaglione+bmo)
https://hg.mozilla.org/mozilla-central/rev/a7b31be0a19a
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Attachment #8759498 - Flags: review?(aswan)
Assignee: nobody → kmaglione+bmo
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.