Closed Bug 1014579 Opened 6 years ago Closed 5 years ago

[RTSP][V2.0] Full screen mode message shows null host when open in a new tab

Categories

(Firefox OS Graveyard :: RTSP, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:2.1+, firefox33 wontfix, firefox34 wontfix, firefox35 fixed, b2g-v2.1 verified, b2g-v2.2 verified)

VERIFIED FIXED
2.1 S7 (24Oct)
blocking-b2g 2.1+
Tracking Status
firefox33 --- wontfix
firefox34 --- wontfix
firefox35 --- fixed
b2g-v2.1 --- verified
b2g-v2.2 --- verified

People

(Reporter: whsu, Assigned: ethan)

References

Details

Attachments

(3 files, 2 obsolete files)

Attached image WP_20140522_005.jpg
* Description:
  After answering an incoming call and resume to fullscreen mode, the warning message shows an inappropriate wording.
  - null is now fullscreen
  Attach the screenshot.(WP_20140522_005.jpg)

* Reproduction steps:
  1. Launch the following page via browser
     - http://goo.gl/FyHFNs
  2. Tap the "Video test page" link
  3. Enter fullscreen mode
  4. Make a phone call to interrupt the RTSP streaming
  5. Resume to fullscreen mode after you hang up the phone.
  6. Checking the warning message of fullscreen mode

* Expected result:
  You can enter full screen mode

* Actual result:
  An inappropriate wording shows on warning message.
  - null is now fullscreen

* Reproduction build: V2.0 (Buri)
 - Gaia      40abb245b1e61df67757ba3747e2f73202e5182b
 - Gecko     https://hg.mozilla.org/mozilla-central/rev/a7d685480bf2
 - BuildID   20140521160200
 - Version   32.0a1
Assignee: nobody → ettseng
William, I could not reproduce this bug.

Moreover, the message should be managed by the browser app.
http://dxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.properties#440

I don't think I am capable of fixing it.
Assignee: ettseng → nobody
(In reply to Ethan Tseng [:ethan] from comment #1)
> William, I could not reproduce this bug.
> 
> Moreover, the message should be managed by the browser app.
> http://dxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/
> browser/browser.properties#440
> 
> I don't think I am capable of fixing it.

Thanks Ethan!
I also cannot reproduce it on the latest V2.0 build.
Some patch might fixed it. I would like to close this bug
If someone can reproduce it, we can reopen this bug.
Thanks!

* Build Information:
 - Gaia      6aa07ea10420bd77f93d7415b5e34d89acc47a7e
 - Gecko     https://hg.mozilla.org/releases/mozilla-aurora/rev/c83fdcf0b735
 - BuildID   20140611160205
 - Version   32.0a2
 => Result: I cannot reproduce
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → WORKSFORME
Status: RESOLVED → VERIFIED
[Blocking Requested - why for this release]:

I encountered the same bug on v2.1.
Attach the screenshot.(2014-07-25-21-08-07.png)

Steps:
1. Launch the browser app
2. Visit following page
   - http://goo.gl/FyHFNs
3. Long tap on the "Video test page"
4. Tap "Open link in new tab"
5. Switch to the new tab page
6. Enable fullscreen mode.
7. Check the message.

* Build information:
 - Gaia      c72257b2d27135bfcd68e89dd584182797784016
 - Gecko     https://hg.mozilla.org/mozilla-central/rev/616e6924cb0b
 - BuildID   20140724160202
 - Version   34.0a1
Status: VERIFIED → REOPENED
blocking-b2g: --- → 2.1?
Resolution: WORKSFORME → ---
Attached image 2014-07-25-21-08-07.png
Hi ettseng, can you help to check it?
Assignee: nobody → ettseng
Following the STRs in comment 3.
If we simply tap the video clip and enable fullscreen. The message is fine.
It would be "https://rawgit.com is now fullscreen."
But if we long tap the video to open the link in a new tab, then the message for fullscreen mode would become "null is now fullscreen".

It seems the subject in this message represents the hostname, and the hostname is not carried to the new tag page.

As I said in comment 1, we have to do to fix this. Need help from browser app team.
Hi Ben,

Could you kindly help to assign someone to look into this issue?
Flags: needinfo?(bfrancis)
change component to browser app based on Comment 6, thanks.
Assignee: ettseng → nobody
Component: RTSP → Gaia::Browser
I think this message is handled by the system app https://github.com/mozilla-b2g/gaia/blob/master/apps/system/locales/system.en-US.properties#L488

Alive, do you know where this code lives and where it gets the origin from?
Component: Gaia::Browser → Gaia::System
Flags: needinfo?(bfrancis) → needinfo?(alive)
Here you are,
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/permission_manager.js#L304

Not sure why do we get a null but this looks like a gecko issue.
Flags: needinfo?(alive)
Assignee: nobody → ettseng
Whiteboard: ft:system-platform
blocking-b2g: 2.1? → 2.1+
Ethan, do you have an update here?
(In reply to Gregor Wagner [:gwagner] from comment #11)
> Ethan, do you have an update here?
Sorry, I was busy working on other issues recently.
I'll get back to investigate this bug and will let you know ASAP.
Thanks.
(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #10)
> Here you are,
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/
> permission_manager.js#L304
> 
> Not sure why do we get a null but this looks like a gecko issue.

After a quick look it seems like the |null| string comes from http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentUtils.cpp#5796 which is called by http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#10852 (then it goes throught all our child process->parent->shell.js->system app pipeline).
[Blocking Requested - why for this release]:

Seems like a Gecko issue to me. Moving to a different component.

That said since this happens only if you open the link directly inside its own tab, and this is a rtsp:// link, I'm a bit surprised that it blocks.

Reasking just in case.
blocking-b2g: 2.1+ → 2.1?
Component: Gaia::System → DOM: Core & HTML
Product: Firefox OS → Core
Gregor marked blocking+ but there's no comment, so requesting ni? to re-evaluate and explain the blocking reason.
Flags: needinfo?(anygregor)
Duplicate of this bug: 1051124
Note - the dupe shows that the reduced STR is actually simple - just request to have a RTSP video be fullscreen.
this is easy to repro, and a silly user error.   ni? jst to help with an owner.   blocking2.1+
blocking-b2g: 2.1? → 2.1+
Flags: needinfo?(jst)
Chris Pearce is a fullscreen expert.
Flags: needinfo?(cpearce)
Flags: needinfo?(jst)
I'd say comment 13 is a rough description of where to start looking.

When we open a video directly, we load a <video> inside an nsVideoDocument in the docshell. I'd guess that's not reporting its origin correctly for some reason.

I'm not in a position where I can afford to work on this at the moment; I've been trying to find someone else to take over fullscreen, but nothing has stuck.
Flags: needinfo?(cpearce)
Whiteboard: ft:system-platform
Modify the summary to reflect the real scenario.
Summary: [RTSP][V2.0] Second time warning message of full screen mode shows an inappropriate wording → [RTSP][V2.0] Full screen mode message shows null host when open in a new tab
Hi Steve,
Using the same protocol flags as HttpHandler in RtspHandler could resolve this issue.
Do you have any concern about it?
Attachment #8500037 - Flags: review?(sworkman)
Additional note.

When loading RTSP in a new tab, VideoDocument::CreateSyntheticVideoDocument() can get the correct 
originalURI from aChannel as loading normally. However, nsContentUtils::GetUTFOrigin() which is called
by nsDocument::RestorePreviousFullScreenState() gets "moz-nullprincipal" as its aURI argument.

After some code trace, I suspect the root cause might be the protocolFlags in RtspHandler.
I didn't find out the point that makes originalURI lost by using the previous flags.
However, I think setting the protocolFlags as HttpHandler should be more appropriate than the previous one.
Comment on attachment 8500037 [details] [diff] [review]
Fix protocol flags of RtspHandler

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

::: netwerk/protocol/rtsp/RtspHandler.cpp
@@ +40,5 @@
>  NS_IMETHODIMP
>  RtspHandler::GetProtocolFlags(uint32_t *aProtocolFlags)
>  {
> +  *aProtocolFlags = URI_STD | ALLOWS_PROXY | ALLOWS_PROXY_HTTP |
> +    URI_LOADABLE_BY_ANYONE;

I think you want to be careful about changing these.

Proxies: do we allow SOCKS or HTTP proxies for RTSP?
NON_PERSISTABLE: that should be enabled, no?

Basically, I'd like some more evidence to show which of these flags is causing the problem.
Attachment #8500037 - Flags: review?(sworkman) → review-
Component: DOM: Core & HTML → RTSP
Product: Core → Firefox OS
Whiteboard: [ETA:10/17]
Target Milestone: --- → 2.1 S7 (Oct24)
Comment on attachment 8500037 [details] [diff] [review]
Fix protocol flags of RtspHandler

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

::: netwerk/protocol/rtsp/RtspHandler.cpp
@@ +40,5 @@
>  NS_IMETHODIMP
>  RtspHandler::GetProtocolFlags(uint32_t *aProtocolFlags)
>  {
> +  *aProtocolFlags = URI_STD | ALLOWS_PROXY | ALLOWS_PROXY_HTTP |
> +    URI_LOADABLE_BY_ANYONE;

Thanks for reminding me to be careful about changing protocol flags.

No, we don't need to allow SOCKS/HTTP proxies for RTSP.
As for URI_NON_PERSISTABLE flag, I think being able to save to a local file would be
a useful feature. Users can save an HTTP streaming link to a local file.
However, that would be another story and I don't tend to change the flag here. :)
Hi Steve,

I located the exact flag -- URI_INHERITS_SECURITY_CONTEXT.
I am sure removing this flag could resolve this bug.
However I'm not sure why and how this flag affects the full-screen warning
message. It seems it's related to principal/security context.

I need more time to investigate it.
But could we change the flag first to resolve this blocker bug?
Attachment #8500037 - Attachment is obsolete: true
Attachment #8504029 - Flags: review?(sworkman)
When the flag URI_INHERITS_SECURITY_CONTEXT is set, the NodePrincipal() in this line
http://dxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#11411
returns moz-nullprincipal and the original URI information is lost.
Comment on attachment 8504029 [details] [diff] [review]
Fix protocol flags of RtspHandler

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

Thanks for looking into this in more detail, Ethan!

I discussed this with some security experts (:tanvi and :geekboy), and this seems right. In general, URIs should inherit when they don't have a hostname/path, so if you look for URI_INHERITS_SECURITY_CONTEXT you'll see it's used for javascript: and data: URIs. So, since RTSP has a hostname/path/etc. it should probably default to not inheriting.

Run it through try and test it manually in different contexts first, just to make sure something else isn't broken because of the change.
Attachment #8504029 - Flags: review?(sworkman) → review+
(In reply to Steve Workman [:sworkman] from comment #28)
> -----------------------------------------------------------------
> Thanks for looking into this in more detail, Ethan!
> I discussed this with some security experts (:tanvi and :geekboy), and this
> seems right. In general, URIs should inherit when they don't have a
> hostname/path, so if you look for URI_INHERITS_SECURITY_CONTEXT you'll see
> it's used for javascript: and data: URIs. So, since RTSP has a
> hostname/path/etc. it should probably default to not inheriting.
Thanks for explaining it, Steve!
It's much more clear to me now. :)
 
> Run it through try and test it manually in different contexts first, just to
> make sure something else isn't broken because of the change.
Sure, I'll do it.
Refresh comment "r=sworkman".
Attachment #8504029 - Attachment is obsolete: true
The result of Treeherder:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ff2b2566b900

Since resolved, the needinfo is not important. Clear it.
Flags: needinfo?(anygregor)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/56330f74fbef
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Comment on attachment 8504479 [details] [diff] [review]
Fix protocol flags of RtspHandler

[Approval Request Comment]
Bug caused by (feature/regressing bug #): None
User impact if declined: Improper wording in warning message before entering
                         full screen mode
Testing completed: On m-c
Risk to taking this patch (and alternatives if risky): None
String or UUID changes made by this patch: None
Attachment #8504479 - Flags: approval-mozilla-b2g34?
Attachment #8504479 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
This issue is verified fixed on Flame 2.1:

Flame 2.1 

Device: Flame 2.1 (319mb)(Kitkat Base)(Full Flash)
BuildID: 20141021001201
Gaia: e458f5804c0851eb4e93c9eb143fe044988cecda
Gecko: ee86921a986f
Gonk: 05aa7b98d3f891b334031dc710d48d0d6b82ec1d
Version: 34.0 (2.1)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

The user can enter fullscreen mode after ending a phone call.
==============================================

Unable to verify on Flame 2.2 due to Bug 1086265. The video controls do not appear when tapping the screen.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Keywords: verifyme
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
This issue is verified fixed on Flame 2.2:

Flame 2.2 

Device: Flame 2.2 Master (319mb)(Kitkat Base)(Full Flash)
BuildID: 20141023040204
Gaia: 27a1d1baaa8e375b70e043efee67d5f2206c330b
Gecko: 88adcf8fef83
Gonk: 05aa7b98d3f891b334031dc710d48d0d6b82ec1d
Version: 36.0a1 (2.2 Master)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

The user can enter fullscreen mode after ending a phone call.
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Keywords: verifyme
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
You need to log in before you can comment on or make changes to this bug.