1438069 - Default "New Tab" in a normal window shows up in a private window

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P5)

Description

[matthewechavez1]

Attachment: Screenshot

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20180208193705

Steps to reproduce:

1. Open a private window
2. Visit a malicious site (I went to weishuapiao.com at first)
3. Proceed to visit that malicious site
4. Click on "Get me out of here!" at the top


Actual results:

My default "New Tab" on a normal window displayed on a private window.


Expected results:

The "Private Browsing with Tracking Protection" window should have displayed.

Comment 1

[Josh Matthews [:jdm]]

That would be the responsibility of getDefaultHomePage: https://searchfox.org/mozilla-central/rev/9011be0a172711bc243e50dfca16d42e877bf4ec/browser/base/content/browser.js#3198-3216

Redirecting to the frontend team, who are in a better position to determine if this is desirable.

Comment 2

[Johann Hofmann [:johannh]]

Sounds like a sensible idea to me, but rather unimportant. This could be a good second bug or something like that. I'm happy to help out whoever wants to take a look at this.

Comment 3

[Arthur Deschamps]

(In reply to Johann Hofmann [:johannh] from comment #2)
> Sounds like a sensible idea to me, but rather unimportant. This could be a
> good second bug or something like that. I'm happy to help out whoever wants
> to take a look at this.

May I try taking a look ?

Comment 4

[Arthur Deschamps]

Attachment: patch.diff

Please, can you guys tell me if my patch looks good to you ?

Comment 5

[Johann Hofmann [:johannh]]

Comment on attachment 8952004 [details] [diff] [review]
patch.diff

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

This looks good, thank you! The safebrowsing pages don't seem to be terribly well covered with tests, but in case you're interested in writing a regression test for this feel free to chat me up on irc.mozilla.org (https://wiki.mozilla.org/IRC) and we can figure it out.

Otherwise I'm happy to accept another patch with the small nit below addressed, I don't think this strictly needs a test.

::: browser/base/content/browser.js
@@ +3201,4 @@
>    var prefs = Services.prefs.getDefaultBranch(null);
>    var url = BROWSER_NEW_TAB_URL;
>    try {
> +    if (!PrivateBrowsingUtils.isWindowPrivate(window))

Nit: you could just write

if (PrivateBrowsingUtils.isWindowPrivate(window))
  return url;

before the try block.

Comment 6

[Arthur Deschamps]

Attachment: patch1.diff

Sorry I wanted to go through mozreview but I must have something messed up with Mercurial because even though I've made a commit, it doesn't detect any changes (thus I can't push review).

Comment 7

[Johann Hofmann [:johannh]]

Comment on attachment 8952454 [details] [diff] [review]
patch1.diff

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

Thanks! I know you're still looking into writing a test, so I'm not checking this in right now. Let me know how that goes :)

Comment 8

[Johann Hofmann [:johannh]]

Hey Arthur, let's check this in to get it into Firefox 60 and solve the tests in a follow-up bug. I'll get this landed now.

Comment 9

[Johann Hofmann [:johannh]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a6faba65ff298d7c628effeb00cefc3b06579322
Bug 1438069 - "Get me out of here" button now redirects to page "Private Browsing with Tracking Protection" in private mode - r=johannh

Comment 10

[Arthur Deschamps]

(In reply to Johann Hofmann [:johannh] from comment #8)
> Hey Arthur, let's check this in to get it into Firefox 60 and solve the
> tests in a follow-up bug. I'll get this landed now.

Yes, sure. I came very short on time lately, but as soon as I can free up some I'll try working on those tests again.

Comment 11

[Cosmin Sabou [:CosminS]]

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