Closed Bug 1191724 Opened 5 years ago Closed 4 years ago

[Private Browser]Open a private page which need login first from normal window, the target private page will be displayed first before loading the login page.

Categories

(Firefox OS Graveyard :: Gaia::Browser, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:2.5+, firefox45 fixed, b2g-v2.2 unaffected, b2g-v2.5 fixed, b2g-master affected)

VERIFIED FIXED
FxOS-S11 (13Nov)
blocking-b2g 2.5+
Tracking Status
firefox45 --- fixed
b2g-v2.2 --- unaffected
b2g-v2.5 --- fixed
b2g-master --- affected

People

(Reporter: huayu.li, Assigned: smaug)

Details

(Keywords: regression, Whiteboard: [2.5-aries-test-run-1][systemsfe])

Attachments

(6 files, 2 obsolete files)

Attached file logcat2.txt
[1.Description]:
[Aries KK v2.5][Flame KK v2.5][Browser]Open a private page which need login first from normal window, the target private page will be displayed first before loading the login page.
Found at:17:03
See attchment:logcat2.txt, Aries3.3gp.

[2.Testing Steps]: 
1. Log in social network (such as facebook) via normal browser 
2. Long press any hyperlink(such as friend profile) which only can be seen if you are login and select to open in private mode 

[3.Expected Result]: 
step2. The target  private window should only show after user login successfully. 

[4.Actual Result]: 
step2. The target page will be displayed first before loading the login page.

[5.Reproduction build]: 

Device: Aries KK 2.5(Affected)
Build ID               20150803195455
Gaia Revision          dbacf8364f4505d021b7d8fb9cabea325004dbcc
Gaia Date              2015-08-03 16:38:49
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/abc56d57f6e1
Gecko Version          42.0a1
Device Name            aries
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20150803.191800
Firmware Date          Mon Aug  3 19:18:08 UTC 2015
Bootloader             s1

Device: Flame KK 2.2(Unaffected)
Build ID               20150805032505
Gaia Revision          f8b119ac30e97df991c97682ac4d4f9ca22e1793
Gaia Date              2015-07-31 13:20:55
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/0c7a85251e10
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150805.065842
Firmware Date          Wed Aug  5 06:58:54 EDT 2015
Firmware Version       v18D v4
Bootloader             L1TC000118D0

Device:Flame KK 2.5(Affected)
Build ID               20150805150207
Gaia Revision          581de383687dc441a878d2c91a0167c6ec688fef
Gaia Date              2015-08-05 01:48:40
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/b12a261ee32e
Gecko Version          42.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150805.195702
Firmware Date          Wed Aug  5 19:57:12 EDT 2015
Firmware Version       v18D v4
Bootloader             L1TC000118D0



[6.Reproduction Frequency]: 
Always Recurrence,5/5

[7.TCID]: 
16498
Attached video Aries3.3gp
QA Contact: ddixon
QA Whiteboard: [COM=Private Browsing]
The oldest build in b2g-inbound / mozilla-inbound is 20150609153752 / 20150609172153, which is later than the first broken build & last working build, so I try to do the regression in Nightly builds.


Nightly Regression Window:
Build ID               20150311010231
Gaia Revision          943c8b4039f59b08ba100390e164a076a20c892e
Gaia Date              2015-03-10 20:35:07
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/fd8e079d6335
Gecko Version          39.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150311.043838
Firmware Date          Wed Mar 11 04:38:50 EDT 2015
Bootloader             L1TC000118D0

First Broken Environmental Variables:
Build ID               20150311170314
Gaia Revision          2b87ee8e7e2ec30a9851b6b59a899006a98767ab
Gaia Date              2015-03-11 02:14:27
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/c33922ee3ac3
Gecko Version          39.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150311.203536
Firmware Date          Wed Mar 11 20:35:47 EDT 2015
Bootloader             L1TC000118D0

First Broken Gaia & Last Working Gecko - issue DOES repro
Gaia Revision          2b87ee8e7e2ec30a9851b6b59a899006a98767ab
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/fd8e079d6335

First Broken Gecko & Last Working Gaia - issue DOES NOT repro
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/c33922ee3ac3
Gaia Revision          943c8b4039f59b08ba100390e164a076a20c892e

Gaia pushlog:
https://github.com/mozilla-b2g/gaia/compare/943c8b4039f59b08ba100390e164a076a20c892e...2b87ee8e7e2ec30a9851b6b59a899006a98767ab
QA Whiteboard: [COM=Private Browsing] → [COM=Private Browsing], [MGSEI-Triage+]
[Blocking Requested - why for this release]:
Regression issue
blocking-b2g: --- → 2.5?
Whiteboard: [2.5-aries-test-run-1] → [2.5-aries-test-run-1][systemsfe]
Nothing really sticks out in the regression range. Michael, can you double check?
Flags: needinfo?(mhenretty)
blocking-b2g: 2.5? → 2.5+
(In reply to Gregor Wagner [:gwagner] from comment #4)
> Nothing really sticks out in the regression range. Michael, can you double
> check?

Don't block on me, whoever picks this bug up should do the investigation.
Flags: needinfo?(mhenretty)
We show the content and login on dekstop as well. Not sure if we should still block on this issue.
(In reply to Gregor Wagner [:gwagner] from comment #6)
> We show the content and login on dekstop as well. Not sure if we should
> still block on this issue.

And even less block in the Gaia:browser component :)

Getting a valuable regression range might be tricky since this looks caching-related.
And FWIW I can't reproduce it with twitter (the private tab has me unlogged from the get-go)
Attachment #8678370 - Attachment is obsolete: true
Something is leaking through, because even after the page loads, I see the "logged-in" sidebar with my account info. (and there's no userID or anything in the URL.)
Stepping through, I can confirm that we set mozprivatebrowsing="true" on the incoming iframe before setting the frame src. I don't see anything wrong on the Gaia side.

I observe the following behavior:

- It is only reproducible on the first load of a given URL; subsequent attempts for the same URL correctly display the "You must log in first" page immediately. Other URLs (other profile pages) similarly fail once, and succeed on subsequent attempts.

- When reproducing, the "wrong" page is shown fully as it loads (i.e. the person's profile page); when the load completes, the page mostly reverts back into a login page -- however, I am still able to see the sidebar that shows my account details. Trying to actually visit any other page results in (correctly) the login page. See a video at <https://youtu.be/3N-GyRpehDE>.

- I grabbed a HTTP log capture using [1], and it appears that when requesting the user's profile page (ostensibly in a private window), we're still sending up a bunch of cookies. (I can send these, and the log, privately if needed.)

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Debugging/HTTP_logging


Based on this, I think we should pass this to the "Private Browsing" component, and potentially unblock (although this issue is somewhat concerning in general). Gregor, what do you think?
Flags: needinfo?(anygregor)
Ehsan, any idea whats going on here? Seems like there is not much we can do from the gaia side.
Flags: needinfo?(anygregor) → needinfo?(ehsan)
I'm not sure if I even understand the bug description.  It seems like this is complaining about some part of the page loading appearing to happen in normal mode, but it's very hard to tell since the reporter is using Facebook where some people have public versions of their profile and it's not clear to me what the reporter expected to see.  Also Facebook is a terrible place to test this in general, it should be easy to write a couple of small test pages, one setting a cookie and the other changing its background color based on the cookie.

At any rate, we don't have this bug in Firefox.  If I'm understanding the report correctly, this is very bad, and it's the kind of thing that can be caused either by a rendering bug (painting the content from the previous page at the bottom) or by a race condition which causes us to set the mozprivatebrowsing attribute after the loading has begun (in which case you should be getting an error reported to the console such as "We should not switch to Private Browsing after loading a document.")  I'm not familiar with the b2g side of things, I think either Fabrice or KanRu may be able to help.
Flags: needinfo?(ehsan)
Additional testing reveals the following:

- This bug only happens when the frame is OOP (remote="true").

- I no longer think this is caching-related; reloading the original page before "open in a new private window" makes this 100% reproducible.

- We're definitely sending cookies up to Facebook from a mozprivatebrowsing="true" frame. I've verified this by clearing cookies and observing IFRAME creation and HTTP logs.

- It does not reproduce when simply copy-pasting the URL into an empty private window, which seems strange to me because the code path for creating that IFRAME is the same as the code path triggered from the context menu.

- I have attempted to isolate the problem in a test case as suggested by Ehsan, but have been unsuccessful in reproducing the problem. Simple cookie tests work as expected.

I realize that a test case would be ideal, but I haven't been able to isolate it any further. Since this only occurs on OOP frames, it seems very much like a platform race; unfortunately, I don't have the expertise to dig deeper without help.
I can't immediately find any code that would make @mozprivatebrowsing work with out of process frames.  Fabrice, you implemented this feature IIRC.  Is this supposed to work with OOP frames?
Flags: needinfo?(fabrice)
We changed the way we set the docshell state in http://hg.mozilla.org/mozilla-central/rev/6e90fd9bf6e2 but this should still work with OOP iframes.

Marcus, do you get a console message like "We should not switch to Private Browsing after loading a document." ?
Flags: needinfo?(fabrice) → needinfo?(m)
No, but I do see:

"Only internal code is allowed to set the usePrivateBrowsing attribute" {file: "about:blank" line: 0}
Flags: needinfo?(m)
That comes from http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#2204

I'm not sure why we emit that unconditionally. Ehsan, do you know?
Flags: needinfo?(ehsan)
It depends on where the message actually comes from; it may or may not be OK.

Marcus, please set a breakpoint on nsDocShell::SetUsePrivateBrowsing() and get a backtrace so that we can tell what is happening.

That being said, it seems like the code for setting the private flag on the docshell has changed tremendously and I haven't reviewed any of the changes so I can't really say how it works any more, or whether it's correct.  :(  Perhaps Josh and/or Olli know more.
Flags: needinfo?(ehsan)
Attached file gdb bt
(In reply to Gregor Wagner [:gwagner] from comment #19)
> Created attachment 8680635 [details]
> gdb bt
Flags: needinfo?(ehsan)
Whoa!  If I understand what this code is doing, this is completely broken.  It seems to me that we are setting the privacy status on the docshell when we're "showing the frame" which, if I understand the meaning of RecvShow() correctly) is way after we have started to load the page.  This is completely unsupported, and the symptoms in comment 0 may be caused by this.  This problem will probably manifest itself in different ways in different websites.

The docshell's privacy state needs to be set soon after it is created, before any content is loaded.  Ideally we should make it work here: <https://dxr.mozilla.org/mozilla-central/rev/1e700005a0ddf2b17803213e1f3f8d78a7a618b8/docshell/base/nsDocShell.cpp#3297>.

Also, private browsing is enabled in 2.5, right?  If yes, we need to disable it immediately, since it's probably completely broken, and we won't have time to fix it.
Flags: needinfo?(ehsan)
Ehsan, why is this not enough to at least know if we started to load the page:
http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#1586 ? Marcus didn't get any console message...

I will double check when TabChild::RecvLoadURL() is called but I'm pretty sure this was after RecvShow() when I landed this patch.
I do recall too that at least originally we did set private browsing mode early enough.
(and doesn't sounds like something which couldn't be fixed for 2.5)
(In reply to [:fabrice] Fabrice Desré from comment #22)
> Ehsan, why is this not enough to at least know if we started to load the
> page:
> http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#1586 ?
> Marcus didn't get any console message...

It _should_ be sufficient, at least assuming that we are checking the correct docshell.  I can't really say for sure without debugging...  :/

> I will double check when TabChild::RecvLoadURL() is called but I'm pretty
> sure this was after RecvShow() when I landed this patch.

Do you know exactly when RecvShow() happens?

The other thing to look into would be why the code I linked to in comment 21 is not sufficient.
(In reply to Olli Pettay [:smaug] from comment #23)
> I do recall too that at least originally we did set private browsing mode
> early enough.

Maybe...  You seem to have reviewed more of this code so you probably know better than I do.

Also I just remembered having heard of a perhaps related issue on e10s.  I'll see if I can find out more about what that was.
Disabling this can not be the option - this is our first "moment in time" release aligning our campaign across all of the mozilla products.
(In reply to Wilfred Mathanaraj [:WDM] from comment #27)
> Disabling this can not be the option

Given that, by when do we need a fix? I've heard 2.5 won't be "released" until a few weeks from now; is that true?
Flags: needinfo?(wmathanaraj)
(In reply to Ehsan Akhgari (don't ask for review please) [Away Nov 3-19] from comment #26)
> Also I just remembered having heard of a perhaps related issue on e10s. 
> I'll see if I can find out more about what that was.

I checked, and that was unrelated.

Really what we should be doing here is to set the privacy state right after we've created the docshell, not at some point later.
fwiw I can't reproduce at all, which doesn't help investigating :(
(In reply to Andrew Overholt [:overholt] from comment #28)
> (In reply to Wilfred Mathanaraj [:WDM] from comment #27)
> > Disabling this can not be the option
> 
> Given that, by when do we need a fix? I've heard 2.5 won't be "released"
> until a few weeks from now; is that true?

RA for 2.5 is tomorrow 10/30/15
I haven't managed to reproduce , but this is possibly a regression from bug 1121905
I think shipping without private browsing is much much better that shipping with a known broken one, if our choices come down to that.
Do we know what it takes to fix/ reproduce this?
I have a possible fix coming, if someone can try a patch.
(just trying to be super careful to not regress anything else so verifying various things, so takes still couple of minutes before I upload it.)
Attached patch possible patch (obsolete) — Splinter Review
So I haven't managed to reproduce this, nor have I ffos dev environment atm, but based on code inspection this might help.
The suspicious code is http://mxr.mozilla.org/mozilla-central/source/dom/base/nsFrameLoader.cpp?rev=e8c7dfe727cd&mark=340-340,342-342#326
So that patch makes parent to always send up-to-date ShowInfo, but child actually cares about the first one.
The sent amount of data isn't much and LoadURL doesn't happen often.

I tried hard to not change name handling (although I think it is wrong on ffos, which is why ApplyShowInfo has IsBrowserOrApp()).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3026b78968d3
Attachment #8680944 - Flags: feedback?(fabrice)
"// Don't allow unsetting private browsing mode." comment is wrong.
Should be
"// No need to re-set private browsing mode."
Attached patch possible patchSplinter Review
Attachment #8680944 - Attachment is obsolete: true
Attachment #8680944 - Flags: feedback?(fabrice)
Attachment #8680948 - Flags: feedback?(fabrice)
Comment on attachment 8680948 [details] [diff] [review]
possible patch

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

That patch works very well for me. Thanks smaug!
Attachment #8680948 - Flags: feedback?(fabrice) → feedback+
Comment on attachment 8680948 [details] [diff] [review]
possible patch

Want to review too? :)
Attachment #8680948 - Flags: review?(fabrice)
(I know the patch is a bit ugly, but we should clean this all up once we clean up also ffos window.name handling.)
Comment on attachment 8680948 [details] [diff] [review]
possible patch

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

lgtm. Can you file the followup for the window.name issues?
Attachment #8680948 - Flags: review?(fabrice) → review+
Attached patch even saferSplinter Review
Just one small tweak, don't change fullscreenallowed handling at all, so
pass false for the fakeShowInfo
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6be160a2587
(In reply to [:fabrice] Fabrice Desré from comment #42)
> 
> lgtm. Can you file the followup for the window.name issues?
https://bugzilla.mozilla.org/show_bug.cgi?id=1219998
The original bug only shows up if the pictures have certain sharing restrictions. I guess only 'share with friends'? I tried the 'even safer' patch and I don't see the content in the new window any more. I get now a message within the facebook page saying: "The page you requested cannot be displayed right now..... or you may not have permission to view this page."

I would say it fixes the bug. \o/
Assignee: nobody → bugs
Attached patch merged to trunkSplinter Review
The patch which moved ::ProvideWindow to ContentChild landed yesterday, so, need to merge with that change.
https://hg.mozilla.org/mozilla-central/rev/b9453b7c7fce
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S11 (13Nov)
Flags: needinfo?(wmathanaraj)
Verified on

[Flame]
Build ID               20151102150204
Gaia Revision          7954ff0cbd794a35499a1082bed273598f82ee6f
Gaia Date              2015-11-02 17:35:17
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/6275cd9c71b76891f6b6585dabc687bc443ab877
Gecko Version          45.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20151102.182914
Firmware Date          Mon Nov  2 18:29:28 EST 2015
Bootloader             L1TC000118D0

[Aries]
Build ID               20151103000930
Gaia Revision          7954ff0cbd794a35499a1082bed273598f82ee6f
Gaia Date              2015-11-02 17:35:17
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/9f69202d82752e093a653a8f15b0274e347db33a
Gecko Version          45.0a1
Device Name            aries
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20151102.233051
Firmware Date          Mon Nov  2 23:30:58 UTC 2015
Bootloader             s1
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.