1562837 - The Full screen button is not displayed when in full screen on HBO GO

Status: VERIFIED FIXED

(Core :: Networking: HTTP, defect, P1)

Description

[Rares Doghi, Desktop QA]

[Affected versions]
Firefox 69.0a1

[Affected platforms]
ALL

[Steps to reproduce]

1. Connect with NORD VPN to a US server.
2. Reach https://play.hbogo.com/ and Login.
3. Start any video and click the Full Screen button.

[Expected result]
The full screen button from the bottom right corner should be displayed.

[Actual result]
The Full screen button dissappears from the bottom right corner.

Regression range pushlog for the first known BAD build:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=4b11fe46132ced08322ca986ec9c56c15e862c06&tochange=251ef3905140484b28314e2dd07b37bc813bab32

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.

Comment 2

[Rares Doghi, Desktop QA]

Attachment: hbogofullscreen.png

Comment 3

[Paul Adenot (:padenot)]

Ehsan, something in bug 1535697 changed this. It's quite strange however, but the regression range and the affected version match up.

What is the process when this happens?

Comment 4

[(no longer active)]

Hi Rares, do you have an HBO Go login that I could use to reproduce this? Does this happen with just a specific movie or with any movie? Thanks!

Comment 5

[(no longer active)]

I can't reproduce this. Here is the exact STR I tried:

1. Update to the latest Nightly on Windows.
2. Use ProtonVPN to connect to a US server.
3. Go to https://play.hbogo.com/ and sign in.
4. Start a few videos and click full screen.

The full screen button is visible before and after going full screen.

I have a few questions:

1. In the screenshot posted, the video is actually not full screen (parts of the page are visible still) so I suspect step 3 of the STR in comment 0 isn't completely accurate. Can you please retest and provide a more accurate STR? How do you make the video go that large but not cover the entire screen? (A screenshot of the entire desktop showing the entire Firefox window would be helpful.)

2. Did you have any other tabs open? Bug 1535697 should really not change anything in a single-tab use case in its expected behaviour.

3. Any chance you could post a screenshot of what you see under Cookies under Content Blocking in the Site Identity panel?

4. Does this reproduce on a new profile?

Thanks!

Comment 6

[Rares Doghi, Desktop QA]

Hi,

1 - In the screenshot posted I managed to reproduce the issue before going to full screen but that only happend on an earlier build while performing the mozregression, I have a attached a new screenshot for the full screen view, its 100% repro on our side we tested on different systems.
2 - The only tab open was the Firefox Privacy Notice that shows when you start Firefox with a fresh profile.
3. - I also attached the screenshot with the cookies from Content Blocking.

One thing to note and I'm not sure that it matters but I was using NordVPN instead of ProtonVPN.

Comment 7

[Rares Doghi, Desktop QA]

Attachment: FULL.png

Comment 8

[Rares Doghi, Desktop QA]

Attachment: Cookies.png

Comment 9

[(no longer active)]

Thanks for the extra info! For some reason now that I tried again to reproduce, I can easily reproduce it, even though this time I see no third-party tracking cookies blocked on the page. Investigating...

Comment 10

[(no longer active)]

I think the bug is caused by this line: https://searchfox.org/mozilla-central/rev/da3f3eaaacb6fb344fd21ac29ace2da0e33f12d3/netwerk/protocol/http/nsHttpConnectionInfo.h#136

It seems like when we call BuildHashKey() there we end up clobbering some of the characters inside the hashkey which are directly stored inside it. For example, we lose the "anonymous" bit since it is set directly in the hash key string rather than stored as a member.

Comment 11

[(no longer active)]

Attachment: Bug 1562837 - Ensure that the fields stored in the nsHttpConnectionInfo hash key are properly preserved when rebuilding it upon a change to the isolation flag;

Comment 12

[(no longer active)]

I have a fix, but unfortunately I'm not sure how to write a test that captures the actual issue, since I didn't manage to figure out the specific core root cause of this. At any rate, manual testing can demonstrate precisely that this patch is enough to fix this bug.

I'm a bit disturbed that 68 will be released without this fix, since I have no idea what the implications of this bug can be elsewhere...

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/598f42e27dfc
Ensure that the fields stored in the nsHttpConnectionInfo hash key are properly preserved when rebuilding it upon a change to the isolation flag; r=michal

Comment 15

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/598f42e27dfc

Comment 16

[Julien Cristau [:jcristau]]

Rares, can you verify this fix in nightly?

(In reply to :Ehsan Akhgari from comment #12)

I'm a bit disturbed that 68 will be released without this fix, since I have no idea what the implications of this bug can be elsewhere...

Ehsan, care to request uplift to beta, and to release and esr68 as well if you believe this is worth including in a dot release?

Comment 17

[(no longer active)]

(In reply to Julien Cristau [:jcristau] from comment #16)

Rares, can you verify this fix in nightly?

(In reply to :Ehsan Akhgari from comment #12)

I'm a bit disturbed that 68 will be released without this fix, since I have no idea what the implications of this bug can be elsewhere...

Ehsan, care to request uplift to beta, and to release and esr68 as well if you believe this is worth including in a dot release?

Yes, I believe this is worth taking in a dot release for sure. I'm waiting for a manual verification before requesting uplift...

Comment 18

[Rares Doghi, Desktop QA]

Hi this issue is verified as Fixed in Firefox Nightly 70.0a1 (2019-07-10) id 20190710094220. On Ubuntu, Mac 10.14 and Windows 10.

Comment 19

[(no longer active)]

Thanks a lot Rares both for verifying and the fantastic bug report! I'm at awe that you found this subtle bug on this site. :-)

Comment 20

[(no longer active)]

Comment on attachment 9076648 [details]
Bug 1562837 - Ensure that the fields stored in the nsHttpConnectionInfo hash key are properly preserved when rebuilding it upon a change to the isolation flag;

Beta/Release Uplift Approval Request

• User impact if declined: Well, obviously comment 0. Also bug 1563550.

But a lot more importantly, this is a core Necko bug, and it's pretty amazing that it hasn't been breaking things in more obvious ways in many other sites. The real outcome of this bug is that we may mistake some Necko HTTP connections internally for entirely unrelated connections, e.g. we may think that a new connection we're just about to open is the same one that we had opened previously, or a duplicate connection which would normally be internally mapped to an existing connection may be sent to the network independently because we may fail to distinguish the correct mapping. The kinds of havoc this may cause totally depends on the site in question. This is a very risky regression to leave unfixed.

• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Comment 0.
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): This patch is changing some core Necko HTTP code, so it is not risk free. That being said, the original regression that bug 1535697 caused here is well understood and based on that the patch is doing the obvious fix without trying to do anything smart or fancy. The patch is "obviously correct" based on a careful audit of the code, but it is difficult to write a good automated test for it because it is not super obvious how to trigger the bad behaviour we were seeing e.g. on HBO Go from the high level at the HTTP channel layer. But I'm confident in that it is a full fix to the regression.

Setting the risk level to medium since touching some C++ code in the guts of Necko isn't entirely risk free, but the risk I'm worried here is more the unknown unknowns...

• String changes made/needed: None

Comment 21

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Comment on attachment 9076648 [details]
Bug 1562837 - Ensure that the fields stored in the nsHttpConnectionInfo hash key are properly preserved when rebuilding it upon a change to the isolation flag;

Fixes a pretty nasty Necko regression. Approved for 69.0b4.

Comment 22

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/36dd2e9d0477

Comment 23

[Rares Doghi, Desktop QA]

This issue is Verified as Fixed in Beta 69.0b4. on Ubuntu, Mac and Windows.

Comment 24

[Julien Cristau [:jcristau]]

Comment on attachment 9076648 [details]
Bug 1562837 - Ensure that the fields stored in the nsHttpConnectionInfo hash key are properly preserved when rebuilding it upon a change to the isolation flag;

necko regression fix, approved for 68.0.1 and 68.1esr

Comment 25

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-release/rev/02a0a0b79539

Comment 26

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-esr68/rev/e8a83ff32123
FIREFOX_ESR_68_0_X_RELBRANCH: https://hg.mozilla.org/releases/mozilla-esr68/rev/4a0435b5a3b0b84398237289cc02f22b15cd22d8

Comment 27

[Rares Doghi, Desktop QA]

This issue is Verified as Fixed in 68.0.1 from https://tools.taskcluster.net/index/gecko.v2.mozilla-release.latest.firefox/win32-opt.
This issue is Verified as Fixed in 68.0.1esr from https://tools.taskcluster.net/index/gecko.v2.mozilla-esr68.latest.firefox/win32-opt.

I will mark it Accordingly.