Closed
Bug 1438069
Opened 6 years ago
Closed 6 years ago
Default "New Tab" in a normal window shows up in a private window
Categories
(Toolkit :: Safe Browsing, defect, P5)
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: matthewechavez1, Assigned: arthur.deschamps1208, Mentored)
References
(Blocks 1 open bug)
Details
(Whiteboard: [lang=js])
Attachments
(2 files, 1 obsolete file)
564.97 KB,
image/png
|
Details | |
956 bytes,
patch
|
johannh
:
review+
|
Details | Diff | Splinter Review |
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•6 years ago
|
||
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
Comment 2•6 years ago
|
||
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
Updated•6 years ago
|
Whiteboard: [lang=js]
Assignee | ||
Comment 3•6 years 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•6 years ago
|
||
Please, can you guys tell me if my patch looks good to you ?
Attachment #8952004 -
Flags: feedback?
Updated•6 years ago
|
Attachment #8952004 -
Flags: feedback? → feedback?(jhofmann)
Updated•6 years ago
|
Assignee: nobody → arthur.deschamps1208
Status: NEW → ASSIGNED
Comment 5•6 years ago
|
||
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•6 years ago
|
||
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 7•6 years ago
|
||
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+
Comment 8•6 years ago
|
||
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•6 years ago
|
||
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•6 years 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•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a6faba65ff29
Status: ASSIGNED → RESOLVED
Closed: 6 years 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.
Description
•