1192120 - Warnings when opening downloads in private browsing

Status: RESOLVED FIXED

(Firefox :: Downloads Panel, defect)

Description

[(no longer active)]

WARNING: content window passed to PrivateBrowsingUtils.isWindowPrivate. Use isContentWindowPrivate instead (but only for frame scripts).
pbu_isWindowPrivate@resource://gre/modules/PrivateBrowsingUtils.jsm:25:14
getData@resource:///modules/DownloadsCommon.jsm:231:9
DownloadsPlacesView@chrome://browser/content/downloads/allDownloadsViewOverlay.js:533:25
init@chrome://browser/content/downloads/contentAreaDownloadsView.js:9:16
onload@about:downloads:1:1
WARNING: content window passed to PrivateBrowsingUtils.isWindowPrivate. Use isContentWindowPrivate instead (but only for frame scripts).
pbu_isWindowPrivate@resource://gre/modules/PrivateBrowsingUtils.jsm:25:14
getIndicatorData@resource:///modules/DownloadsCommon.jsm:253:9
DownloadsPlacesView@chrome://browser/content/downloads/allDownloadsViewOverlay.js:538:3
init@chrome://browser/content/downloads/contentAreaDownloadsView.js:9:16
onload@about:downloads:1:1
WARNING: content window passed to PrivateBrowsingUtils.isWindowPrivate. Use isContentWindowPrivate instead (but only for frame scripts).
pbu_isWindowPrivate@resource://gre/modules/PrivateBrowsingUtils.jsm:25:14
init@chrome://browser/content/downloads/contentAreaDownloadsView.js:11:10
onload@about:downloads:1:1

Comment 1

[(no longer active)]

Attachment: Fix warnings that show up when opening the downloads window in private browsing mode

Comment 2

[(no longer active)]

Attachment: Fix warnings that show up when opening the downloads window in private browsing mode

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8644773 [details] [diff] [review]
Fix warnings that show up when opening the downloads window in private browsing mode

Review of attachment 8644773 [details] [diff] [review]:
-----------------------------------------------------------------

I think you're missing one more in DownloadsCommon.jsm, in getSummary.

r=me with that fixed.

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae5f5a18ddd0

Comment 5

[:Paolo Amadini]

Comment on attachment 8644773 [details] [diff] [review]
Fix warnings that show up when opening the downloads window in private browsing mode

Review of attachment 8644773 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/downloads/DownloadsCommon.jsm
@@ +227,5 @@
>     * @param aWindow
>     *        The browser window which owns the download button.
>     */
>    getData(aWindow) {
> +    if (PrivateBrowsingUtils.isContentWindowPrivate(aWindow)) {

Hm, why are we using PrivateBrowsingUtils.isContentWindowPrivate if aWindow is a browser XUL window?

Comment 6

[:Paolo Amadini]

In fact, looks like DownloadsCommon isn't in the warning stacks.

Comment 7

[:Paolo Amadini]

Nevermind, didn't see it at first.

Comment 8

[:Paolo Amadini]

So, it looks like the argument to the DownloadsCommon functions can be the browser window or the content window for the in-content views.

Comment 9

[(no longer active)]

Hmm, does that mean that my patch is wrong?

Comment 10

[Phil Ringnalda (:philor)]

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

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Looking at the code in PrivateBrowsingUtils, it looks like we actually converging on the same codepath that we were before (using this.privateContextFromWindow(aWindow).usePrivateBrowsing).

The only difference is that it looks like isContentWindowPrivate has documentation that states that that method is only to be used from framescripts.

Why is that requirement there, ehsan?

Comment 12

[(no longer active)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #11)
> Looking at the code in PrivateBrowsingUtils, it looks like we actually
> converging on the same codepath that we were before (using
> this.privateContextFromWindow(aWindow).usePrivateBrowsing).
> 
> The only difference is that it looks like isContentWindowPrivate has
> documentation that states that that method is only to be used from
> framescripts.
> 
> Why is that requirement there, ehsan?

Bill added it in bug 1069059 and you reviewed.  I was never consulted.  :-)

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Irony intensifying.

Comment 14

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

Well, the rules are supposed to be as follows:
- If you're in a frame script, use isContentWindowPrivate.
- If you're not, and you have a <browser> element, use isBrowserPrivate.
- Otherwise, you need to have a chrome window and use isWindowPrivate.
The idea is to avoid people passing around generic DOM windows without really knowing whether it's a content window or a chrome window since that style is really bad for e10s.

In this case, about:downloads is a content DOM window that's loaded in the chrome process, which the rules weren't really designed for. I guess using isContentWindowPrivate is okay. However, I still feel like it would be nicer to pass something different around than a DOM window. In the code I've looked at so far, it seems like it would be pretty easy to find the <browser> element or the XUL window. The <browser> element is a little nicer in case we ever want to do per-tab private browsing I guess (people have told me that's something we might want to do).

Comment 15

[(no longer active)]

I don't think we have any plans for ever doing a per-tab PB mode.

Besides that, is there anything else to be done in this bug?

Comment 16

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

(In reply to Ehsan Akhgari (don't ask for review please) from comment #15)
> I don't think we have any plans for ever doing a per-tab PB mode.

OK, good. It's much easier to just check the private browsing state on the window.

> Besides that, is there anything else to be done in this bug?

No, guess not.

Comment 17

[(no longer active)]

Thanks!