Closed
Bug 1191724
Opened 9 years ago
Closed 9 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)
Tracking
(blocking-b2g:2.5+, firefox45 fixed, b2g-v2.2 unaffected, b2g-v2.5 fixed, b2g-master affected)
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)
282.40 KB,
text/plain
|
Details | |
7.57 MB,
video/3gpp
|
Details | |
6.09 KB,
text/plain
|
Details | |
14.44 KB,
patch
|
fabrice
:
review+
fabrice
:
feedback+
|
Details | Diff | Splinter Review |
14.41 KB,
patch
|
Details | Diff | Splinter Review | |
14.83 KB,
patch
|
Details | Diff | Splinter Review |
[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
Reporter | ||
Comment 1•9 years ago
|
||
Reporter | ||
Updated•9 years ago
|
Updated•9 years ago
|
Keywords: regressionwindow-wanted
Updated•9 years ago
|
QA Contact: ddixon
Updated•9 years ago
|
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+]
Keywords: regressionwindow-wanted
Comment 3•9 years ago
|
||
[Blocking Requested - why for this release]:
Regression issue
blocking-b2g: --- → 2.5?
Updated•9 years ago
|
Whiteboard: [2.5-aries-test-run-1] → [2.5-aries-test-run-1][systemsfe]
Comment 4•9 years ago
|
||
Nothing really sticks out in the regression range. Michael, can you double check?
Flags: needinfo?(mhenretty)
Updated•9 years ago
|
blocking-b2g: 2.5? → 2.5+
Comment 5•9 years ago
|
||
(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)
Comment 6•9 years ago
|
||
We show the content and login on dekstop as well. Not sure if we should still block on this issue.
Comment 7•9 years ago
|
||
(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)
Comment 8•9 years ago
|
||
Updated•9 years ago
|
Attachment #8678370 -
Attachment is obsolete: true
Comment 9•9 years ago
|
||
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.)
Comment 10•9 years ago
|
||
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)
Comment 11•9 years ago
|
||
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)
Comment 12•9 years ago
|
||
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)
Comment 13•9 years ago
|
||
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.
Comment 14•9 years ago
|
||
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)
Comment 15•9 years ago
|
||
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)
Comment 16•9 years ago
|
||
No, but I do see:
"Only internal code is allowed to set the usePrivateBrowsing attribute" {file: "about:blank" line: 0}
Flags: needinfo?(m)
Comment 17•9 years ago
|
||
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)
Comment 18•9 years ago
|
||
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)
Comment 19•9 years ago
|
||
Comment 20•9 years ago
|
||
(In reply to Gregor Wagner [:gwagner] from comment #19)
> Created attachment 8680635 [details]
> gdb bt
Flags: needinfo?(ehsan)
Comment 21•9 years ago
|
||
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)
Comment 22•9 years ago
|
||
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.
Assignee | ||
Comment 23•9 years ago
|
||
I do recall too that at least originally we did set private browsing mode early enough.
Assignee | ||
Comment 24•9 years ago
|
||
(and doesn't sounds like something which couldn't be fixed for 2.5)
Comment 25•9 years ago
|
||
(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.
Comment 26•9 years ago
|
||
(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.
Comment 27•9 years ago
|
||
Disabling this can not be the option - this is our first "moment in time" release aligning our campaign across all of the mozilla products.
Comment 28•9 years ago
|
||
(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)
Comment 29•9 years ago
|
||
(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.
Comment 30•9 years ago
|
||
fwiw I can't reproduce at all, which doesn't help investigating :(
Comment 31•9 years ago
|
||
(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
Assignee | ||
Comment 32•9 years ago
|
||
I haven't managed to reproduce , but this is possibly a regression from bug 1121905
Comment 33•9 years ago
|
||
I think shipping without private browsing is much much better that shipping with a known broken one, if our choices come down to that.
Comment 34•9 years ago
|
||
Do we know what it takes to fix/ reproduce this?
Assignee | ||
Comment 35•9 years ago
|
||
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.)
Assignee | ||
Comment 36•9 years ago
|
||
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)
Assignee | ||
Comment 37•9 years ago
|
||
"// Don't allow unsetting private browsing mode." comment is wrong.
Should be
"// No need to re-set private browsing mode."
Assignee | ||
Comment 38•9 years ago
|
||
Attachment #8680944 -
Attachment is obsolete: true
Attachment #8680944 -
Flags: feedback?(fabrice)
Attachment #8680948 -
Flags: feedback?(fabrice)
Comment 39•9 years ago
|
||
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+
Assignee | ||
Comment 40•9 years ago
|
||
Comment on attachment 8680948 [details] [diff] [review]
possible patch
Want to review too? :)
Attachment #8680948 -
Flags: review?(fabrice)
Assignee | ||
Comment 41•9 years ago
|
||
(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 42•9 years ago
|
||
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+
Assignee | ||
Comment 43•9 years ago
|
||
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
Assignee | ||
Comment 44•9 years ago
|
||
(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
Comment 45•9 years ago
|
||
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/
Updated•9 years ago
|
Assignee: nobody → bugs
Assignee | ||
Comment 46•9 years ago
|
||
The patch which moved ::ProvideWindow to ContentChild landed yesterday, so, need to merge with that change.
Comment 47•9 years ago
|
||
Comment 48•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S11 (13Nov)
Updated•9 years ago
|
Flags: needinfo?(wmathanaraj)
Comment 49•9 years ago
|
||
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
Comment 50•9 years ago
|
||
bugherder uplift |
status-b2g-v2.5:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•