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

RESOLVED FIXED in Firefox 60

Status

()

P5
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: matthewechavez1, Assigned: arthur.deschamps1208, Mentored)

Tracking

(Blocks: 1 bug)

59 Branch
mozilla60
Points:
---

Firefox Tracking Flags

(firefox60 fixed)

Details

(Whiteboard: [lang=js])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

a year ago
Posted image 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.

Updated

a year ago
Component: Untriaged → Private Browsing
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.
Status: UNCONFIRMED → NEW
Component: Private Browsing → General
Ever confirmed: true
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.
Mentor: jhofmann
Component: General → Safe Browsing
Priority: -- → P5
Product: Firefox → Toolkit
Whiteboard: [lang=js]
(Assignee)

Comment 3

a year ago
(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 ?
(Assignee)

Comment 4

a year ago
Posted patch patch.diff (obsolete) — Splinter Review
Please, can you guys tell me if my patch looks good to you ?
Attachment #8952004 - Flags: feedback?
Attachment #8952004 - Flags: feedback? → feedback?(jhofmann)
Assignee: nobody → arthur.deschamps1208
Status: NEW → ASSIGNED
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.
Attachment #8952004 - Flags: feedback?(jhofmann) → feedback+
(Assignee)

Comment 6

a year ago
Posted patch patch1.diffSplinter Review
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).
Attachment #8952004 - Attachment is obsolete: true
Attachment #8952454 - Flags: review?(jhofmann)
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 :)
Attachment #8952454 - Flags: review?(jhofmann) → review+
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.
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
(Assignee)

Comment 10

a year ago
(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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a6faba65ff29
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.