Closed Bug 1562837 Opened 2 months ago Closed Last month

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

Categories

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

Desktop
All
defect

Tracking

()

VERIFIED FIXED
mozilla70
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 68+ verified
firefox67 --- unaffected
firefox68 + verified
firefox69 + verified
firefox70 --- verified

People

(Reporter: rares.doghi, Assigned: Ehsan)

References

(Regression)

Details

(Keywords: regression, Whiteboard: [necko-triaged])

Attachments

(4 files)

[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

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

Attached image hbogofullscreen.png
Summary: The Full screen button is not displayed in full screen on HBO GO → The Full screen button is not displayed when in full screen on HBO GO

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?

Flags: needinfo?(ehsan)

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!

Flags: needinfo?(rares.doghi)
Has Regression Range: --- → yes
Has STR: --- → yes
Component: Audio/Video: Playback → Networking: HTTP
Regressed by: 1535697

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!

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.

Flags: needinfo?(rares.doghi)
Attached image FULL.png
Attached image Cookies.png

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...

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.

Flags: needinfo?(ehsan)

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...

Assignee: nobody → ehsan
Duplicate of this bug: 1563550
Whiteboard: [necko-triaged]
Priority: -- → P1
Pushed by eakhgari@mozilla.com:
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
Status: NEW → RESOLVED
Closed: Last month
Resolution: --- → FIXED
Target Milestone: --- → mozilla70

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?

Flags: qe-verify+
Flags: needinfo?(rares.doghi)
Flags: needinfo?(ehsan)

(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...

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.

Flags: needinfo?(rares.doghi)

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. :-)

Flags: needinfo?(ehsan)

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

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
Attachment #9076648 - Flags: approval-mozilla-release?
Attachment #9076648 - Flags: approval-mozilla-beta?

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.

Attachment #9076648 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
QA Whiteboard: [qa-triaged]

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

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

Attachment #9076648 - Flags: approval-mozilla-release?
Attachment #9076648 - Flags: approval-mozilla-release+
Attachment #9076648 - Flags: approval-mozilla-esr68+

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.

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.