1317173 - [non-e10s] Pop-ups opened in Private Browsing mode stay in history

Status: VERIFIED FIXED

(Firefox :: Private Browsing, defect, P1)

Description

[zorzi]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20160921204512

Steps to reproduce:

I accessed to a site (cam4.fr) in private-navigation mode.


Actual results:

Clicking on some links openened a popup. After closing Firefox, this popup stay in history in spite the fact that it occured in private navigation mode.


Expected results:

No appearance of visited websites in private navigation in Firefox history

Comment 1

[Loic]

I tried this NSFW website, some ad pop-ups are open when clicking on a random model webcam but they are open in private mode like the original tab.

Can you test with FF49 and a fresh profile?
https://support.mozilla.org/en-US/kb/profile-manager-create-and-remove-firefox-profiles

Comment 2

[zorzi]

Hello,

I checked again after deleting my profile.

The problem seems to be linked to the Ublock extension. When I disable Ublock, the problem seems to vanish.

Comment 3

[zorzi]

I should have precised that I'm using Ublock 1.9.16.

Comment 4

[Loic]

I don't know Ublock, but does this add-on have settings about PB mode?

Did it use to work in previous versions of Firefox with this add-on installed?

Comment 5

[zorzi]

Can't find Ublock settings that would allow to be disabled in PB mode.

Problem is the same with Pale Moon (the Firefox 24 fork) + Ublock.

Shall try another adblocker.

Comment 6

[u287251]

uBlock Origin author here.

This is not caused by uBlock Origin -- this is what appear to be a normal behavior by the browser. The lack of exact details by OP is not helping.

> When I disable Ublock, the problem seems to vanish.

I do see entries for `cam4.fr` in History's "Recently closed tabs" list. 

However:

- This also happen WITHOUT uBlock Origin.

- These entries are only present in the private window, not in the non-private windows. Once the private window is closed, there is no trace in History that the site was ever visited.

- These entries are those of closed tabs. Without uBO installed, if one close a tab manually (rather than uBO doing through its popup blocker), the entries also appear under "Recently closed tabs".

- This is true of FF 45 and FF 49.

So I do not see any issue, with uBO or with Firefox: whatever is added in the History panel while in private mode is not there in non-private mode, i.e. once the private window is closed, there is no trace a user visited cam4.fr. The entries added are those of the recently closed tabs, so my understanding is it is by design -- and completely unrelated to uBO.

Comment 7

[u287251]

Maybe duplicate of:

- https://bugzilla.mozilla.org/show_bug.cgi?id=828812
- https://bugzilla.mozilla.org/show_bug.cgi?id=828812

?

The only difference is that uBO may automatically close nuisance popups (tab- or window-based), whereas in the issues listed above the user closed the tab manually.

If not this, then I was unable to reproduce OP's "[a]fter closing Firefox, this popup stay in history".

uBO does not play with any history API, at most it programmatically closes tab- or window-based popups according to the content of filter lists.

Comment 8

[u287251]

One of the link above should be:

- https://bugzilla.mozilla.org/show_bug.cgi?id=503872

I pasted twice the same link by mistake.

Comment 9

[u287251]

I finally could reproduce the issue, with Pale Moon however. I have a set of steps with which I can reproduce every time, but this requires dealing with the NSFW site used by OP. Now the steps work every time on my side with the debian version of Pale Moon 26.2.2 -- I haven't been able to reproduce so far with Firefox 45.4.0, I suspect there might be some timing issues going on. I will keep investigating further to gather as much details as possible.

The steps which works for me every time:
- Launch Pale Moon.
- Have uBlock Origin 1.9.16 installed. Default settings + all lists up to date.
- Open a "New Private Window" from "File" menu.
- In the new private window, type "cam4.fr" in address bar.
- A page with various thumbnails will open.
- Click the thumbnail "Sextwoo"
- A separate popup window will open, while at the same time the current tab in the private window navigate to the clicked link.
- The new popup window is blank
    - The script which it is supposed to load is blocked by uBO -- which is why the page is blank.
    - The URL of the popup window is "http://en.cam4.fr/ads/pops/popunder.jsp"
    - It is this exact URL which is added to the history

Now I can't reproduce it with FF 45. I will try more, and will try to further narrow what is needed or not for this to happen.

Comment 10

[u287251]

Need to correct myself regarding my last comment: I can reproduce the issue with this exact scenario for Firefox 45.4.0. So far:

Pale Moon and Firefox 45.4 exhibit the issue. Firefox 49.0.2 and Nightly do not exhibit the issue. The issue is a bit difficult to reproduce because it seems these exact popup/popunder must be launched by the page for it to occur. Will keep investigating.

Comment 11

[u287251]

I confirm this occurs as a result of uBO's popunder filtering. If the original tab is not closed by uBO as a result of popunder filtering, the issue does not occur[1].

The way uBO closes popunder tabs is no different than the way it closes popup tabs. However it seems programmatically closing a popunder in the above scenario triggers some specific code paths in Firefox 45 leading to the history leak issue.

[1] Using this custom filter prevent the issue from occurring: @@||advertiserurl.com^$third-party,popup

Comment 12

[zorzi]

Thanks for investigating.

Comment 13

[u287251]

I can confirm this is a Firefox issue, I wrote a minimal test case (hosted on my personal server: raymondhill.net) which reproduce the issue with a plain Firefox profile with no extension running.

Repro steps:

1. Launch Firefox 45 ESR with a new profile.
2. Uncheck "Block popup windows" in Preferences/Content.
3. Launch a new "Create Private Window" (following steps are done with this new private window).
4. Navigate to http://www.raymondhill.net/ublock/bugzilla1317173/index.html
5. Click the link "Launch popup tab/window"

Result: a new tab is created and a new popup window appears.

6. Close the ORIGINAL (important!) tab, close the popup window.

Result: You should be left with only one tab, which is actually the same as the one originally opened. This first step is needed for this specific repro case because window.close() does not work on a tab NOT opened programmatically. Now we have a tab opened programmatically, thus it will be able to close itself.

7. Click the link "Launch popup tab/window"

Result: a new tab is created and a new popup window appears. Now observe that the URL of the popup window has leaked into History.

So it appears there is a timing issue going on for when a tab launches a popup window while shortly thereafter this "parent" tab is closed.

Though the issue is with Firefox, I have created a workaround in uBO's code to avoid the leaking, by delaying the closing of tabs. The delay is kind of arbitrary (currently I have observed 250ms works).

This Firefox issue manifested itself with uBO because of its popunder-blocking feature, which as far as I know is not supported by other blockers.

There is only two very small files for the repro case, they can be hosted anywhere. Here they are:

index.html:

    <!DOCTYPE html>
    <html>
    <head>
    <title>Good</title>
    </head>
    <body>
    <a href="index.html">Launch popup tab/window</a>
    <script>
    var a = document.querySelector('a');
    a.addEventListener('click', function() {
        window.open('index.html');
        setTimeout(function() {
            window.close();
        }, 1);
        window.open('leak.html', 'leak', 'menubar=no,location=no,resizable=no,scrollbars=no,status=no,toolbar=no');
    });
    </script>
    </body>
    </html>

leak.html:

    <!DOCTYPE html>
    <html>
    <head>
    <title>Leak into history!</title>
    </head>
    <body>
    Leak into history!
    </body>
    </script>
    </html>

Comment 14

[Loic]

Ok, I can reproduce the issue with your testcase in FF50/53. The funny thing is e10s is not affected, only non-e10s mode.

I tested the old version FF33, same issue about private URL leaking in history.

Comment 15

[Loic]

Ehsan, can you find someone at Mozilla who can investigate this issue, please.
Raymond Hill has provided a simplified testcase and STR in comment #13.

Comment 16

[(no longer active)]

Thanks a lot Raymond, I could easily reproduce this with the instructions in comment 13 in non-e10s mode.

baku, do you have cycles to look into this?

Comment 17

[Andrea Marchesini [:baku]]

Attachment: pb.patch

In this test we execute the ::Close() before ::Open(). This happens because:

#0  mozilla::dom::WindowBinding::close (cx=0x7fff484b4d68, obj=<error reading variable: Cannot access memory at address 0x0>, self=0x7fff484b4b10, args=...)
...
#13 0x00007f10f83e689b in mozilla::dom::Function::Call<nsCOMPtr<nsISupports> > (this=0x606000330e60, thisVal=..., arguments=..., aRetVal=..., aRv=..., 
    aExecutionReason=0x7f1106eafea0 <.str.545> "setTimeout handler", aExceptionHandling=mozilla::dom::CallbackObject::eReportExceptions, aCompartment=0x0)
    at /home/baku/Sources/m/bugs2/build/dist/include/mozilla/dom/FunctionBinding.h:70
....
#17 0x00007f10f4168db1 in nsTimerImpl::Fire (this=0x608000184e20) at /home/baku/Sources/m/bugs2/src/xpcom/threads/nsTimerImpl.cpp:477
...
#21 0x00007f10f410bb58 in nsThread::ProcessNextEvent (this=0x6130000129c0, aMayWait=true, aResult=0x7fff484c5740) at /home/baku/Sources/m/bugs2/src/xpcom/threads/nsThread.cpp:1213
#22 0x00007f10f422ff67 in NS_ProcessNextEvent (aThread=0x6130000129c0, aMayWait=true) at /home/baku/Sources/m/bugs2/src/xpcom/glue/nsThreadUtils.cpp:361
#23 0x00007f10ffdc6ff8 in nsXULWindow::CreateNewContentWindow (this=0x61200029eb40, aChromeFlags=1230, aOpeningTab=0x0, aOpener=0x6190013371a0, _retval=0x7fff484c5de0)
    at /home/baku/Sources/m/bugs2/src/xpfe/appshell/nsXULWindow.cpp:2022
...
#28 0x00007f10ffc8cc27 in nsWindowWatcher::OpenWindowInternal (this=0x60800004eea0, aParent=0x6190013371a0, aUrl=0x603000972cd8 "leak.html", aName=0x7fff484c86c0 "leak", 
    aFeatures=0x60d0002cac38 "menubar=no,location=no,resizable=no,scrollbars=no,status=no,toolbar=no", aCalledFromJS=true, aDialog=false, aNavigate=true, aArgv=0x0, aIsPopupSpam=false, aForceNoOpener=false, 
    aLoadInfo=0x0, aResult=0x7fff484c85c0) at /home/baku/Sources/m/bugs2/src/embedding/components/windowwatcher/nsWindowWatcher.cpp:1003
...
#34 0x00007f10f837faf0 in nsGlobalWindow::Open (this=0x619000537580, aUrl=..., aName=..., aOptions=..., aError=...) at /home/baku/Sources/m/bugs2/src/dom/base/nsGlobalWindow.cpp:8279

Doing this, mDocShell of the current window is set to null. There are many ways we can fix this issue:

. we could postpone the close() in case we have an open() in stack
. we could use subectPrincipal if we don't have parent nsILoadContext (taken from the nullified mDocShell)

I did the latter in this patch.

Probably we should check if we really need all of this:

https://dxr.mozilla.org/mozilla-central/source/embedding/components/windowwatcher/nsWindowWatcher.cpp#1148-1163

maybe we should change this so that, if chromeFlags contains CHROME_PRIVATE_WINDOW or CHROME_NON_PRIVATE_WINDOW we force the new privateBrowsing flag. Otherwise we do nothing (the correct value should be already taken from the subjectPrincipal).

Comment 18

[Boris Zbarsky [:bzbarsky]]

Well, for a start we should suspend the parent window before spinning the event loop just like we'd do for a sync XHR.  The fact that the timer fires before the open() call returns is just wrong.

Past that, we should snapshot whatever state we need from the opener window before we start calling into things like window providers and window creators, since we don't really control what they will do and they can in fact tear down that parent window even if it's suspended.

Comment 19

[Tanvi Vyas[:tanvi]]

This bug is an old bug that will need two patches.  One for ESR/before OriginAttributes when we use the private browsing boolean, and one for after with Origin Attribute private mode flag.

Comment 20

[Andrea Marchesini [:baku]]

Attachment: pb.patch

This bug goes away if we suspend the parent window before creating the new one.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8816228 [details] [diff] [review]
pb.patch

This is still just a partial fix.
There might be some incoming network events, like async XHR' load event or WebSocket's message which might call .close() at wrong time.

I think we want this, but we also must fix the pb flag propagation.
If you land this patch first, open a new bug to fix the issue for good.
Or wait for landing this patch until there is also a patch for pb flag propagation.
So, I agree with comment 18.

Comment 22

[Andrea Marchesini [:baku]]

Comment on attachment 8814182 [details] [diff] [review]
pb.patch

What about having also this patch for the PB propagation?

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8814182 [details] [diff] [review]
pb.patch

Shouldn't we just read the pb status from parent earlier and use that.
Basically what comment 18 says.

Comment 24

[Andrea Marchesini [:baku]]

Attachment: pb2.patch

Comment 25

[Andrea Marchesini [:baku]]

Attachment: pb2.patch

Comment 26

[Andrea Marchesini [:baku]]

Attachment: pb2.patch

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8817185 [details] [diff] [review]
pb2.patch

>+      isPrivateBrowsingWindow = fals;
This doesn't compile, so I assume not tested ;)

And don't you change the meaning of CHROME_NON_PRIVATE_WINDOW.
If both nsIWebBrowserChrome::CHROME_PRIVATE_WINDOW and nsIWebBrowserChrome::CHROME_NON_PRIVATE_WINDOW are set, isPrivateBrowsingWindow should be false.

Comment 28

[Andrea Marchesini [:baku]]

> This doesn't compile, so I assume not tested ;)

I submitted twice the same patch. Forgetting to do hg qref. The patch has been tested.

Comment 29

[Andrea Marchesini [:baku]]

Attachment: pb2.patch

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/991e29b1efd2
Parent window should be frozen when a new child window is opened, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3945af43157
Better propagation of private browsing flag when a new window is opened, r=smaug

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/991e29b1efd2
https://hg.mozilla.org/mozilla-central/rev/a3945af43157

Comment 32

[Tanvi Vyas[:tanvi]]

Do we need uplifts here?

Comment 33

[Andrea Marchesini [:baku]]

Comment on attachment 8816228 [details] [diff] [review]
pb.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: See the aurora/beta uplift comment
Fix Landed on Version: current nightly
Risk to taking this patch (and alternatives if risky): see below
String or UUID changes made by this patch: none

Approval Request Comment
[Feature/Bug causing the regression]: PrivateBrowsing
[User impact if declined]: there is a way to exit from the private Browsing sandbox in non-e10s mode
[Is this code covered by automated tests?]: not yet
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]:  STR is included in comment 13
[List of other uplifts needed for the feature/fix]: just these 2 patches
[Is the change risky?]: no
[Why is the change risky/not risky?]: This patch suspend/resume the parent window when a new one is opened. The patch is very simple and the same pattern is used elsewhere (for instance in sync XHR).
[String changes made/needed]: none

Comment 34

[Andrea Marchesini [:baku]]

Comment on attachment 8817242 [details] [diff] [review]
pb2.patch

[Why is the change risky/not risky?]: The current way to set privateBrowsing flag is buggy. This patch takes the correct value to set from the OriginAttributes of the subjectPrincipal in case no other flags have been set.

Comment 35

[Gerry Chang [:gchang]]

Hi Florin,
Can you help find someone in your team to verify if this is fixed based on comment #13?

Comment 36

[Gerry Chang [:gchang]]

Comment on attachment 8816228 [details] [diff] [review]
pb.patch

Fix an issue related to exit from the private Browsing sandbox. Beta51+ & Aurora52+. Should be in 51 beta 8.

Comment 37

[Alexandru Simonca [:asimonca], Release Desktop QA]

This issue is VERIFIED FIXED in Firefox Nightly 53.0a1 (id: 20161214030231) and also in Firefox Aurora 52.0a2 (id: 20161215004017).
Please ni? me for further assistance.

Comment 38

[Carsten Book [:Tomcat]]

needs rebasing for aurora

Comment 39

[Andrea Marchesini [:baku]]

Attachment: part 2 for m-a

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/21b41507b2e5
https://hg.mozilla.org/releases/mozilla-aurora/rev/123b0bf81002

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9293ce14dd71
https://hg.mozilla.org/releases/mozilla-beta/rev/c63d33a0f879

Comment 42

[Ryan VanderMeulen [:RyanVM]]

Backed out from beta for bustage.
https://hg.mozilla.org/releases/mozilla-beta/rev/28be1a348351539bd5341c44120a0829d21a7a35

https://treeherder.mozilla.org/logviewer.html#?job_id=2091131&repo=mozilla-beta

/builds/slave/m-beta-m64-add-on-devel-000000/build/src/embedding/components/windowwatcher/nsWindowWatcher.cpp:995:31: error: no member named 'Suspend' in 'nsPIDOMWindowInner'
/builds/slave/m-beta-m64-add-on-devel-000000/build/src/embedding/components/windowwatcher/nsWindowWatcher.cpp:1035:31: error: no member named 'Resume' in 'nsPIDOMWindowInner'

Comment 43

[Liz Henry (:lizzard) (relman/hg->git project)]

baku, any luck here? Do you need to rebase for beta? 

At this point with ESR, we are nearly at the end of the year-long cycle and will release ESR52 in early March. We limit uplifts to sec-critical and sec-high issues. I don't think we should include this in ESR 45.7.0 in January.

Comment 44

[Andrea Marchesini [:baku]]

Attachment: patch 1 - m-b

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ffc99a75ab86
https://hg.mozilla.org/releases/mozilla-beta/rev/1d09fe652dd4

Comment 46

[Ryan VanderMeulen [:RyanVM]]

Backed out from Beta for hitting widespread assertions.
https://treeherder.mozilla.org/logviewer.html#?job_id=68565648&repo=mozilla-beta

https://hg.mozilla.org/releases/mozilla-beta/rev/eae3062829b3

Comment 47

[Emil Ghitta, QA [:emilghitta]]

I've reproduced the issue described in comment 13 using 45.4.0 ESR (Build Id:20160905130425) and verified that the issues is not reproducible anymore using Firefox 52 beta 8 (Build ID:20170220070057) and latest Developer Edition 53.0a2 (Build ID: 20170221004019) across platforms (macOS 10.12.3, Ubuntu 14.04 32bit, Windows 10 64bit).