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.

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.

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

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

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

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.

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.

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

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

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

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