Closed
Bug 1014579
Opened 11 years ago
Closed 10 years ago
[RTSP][V2.0] Full screen mode message shows null host when open in a new tab
Categories
(Firefox OS Graveyard :: RTSP, defect)
Tracking
(blocking-b2g:2.1+, 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)
491.68 KB,
image/jpeg
|
Details | |
152.56 KB,
image/png
|
Details | |
1.12 KB,
patch
|
bajaj
:
approval-mozilla-b2g34+
|
Details | Diff | Splinter Review |
* 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 | ||
Updated•11 years ago
|
Assignee: nobody → ettseng
Assignee | ||
Comment 1•11 years ago
|
||
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
Reporter | ||
Comment 2•11 years ago
|
||
(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: 11 years ago
Resolution: --- → WORKSFORME
Reporter | ||
Updated•11 years ago
|
Status: RESOLVED → VERIFIED
Reporter | ||
Comment 3•11 years ago
|
||
[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 → ---
Reporter | ||
Comment 4•11 years ago
|
||
Assignee | ||
Comment 6•11 years ago
|
||
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.
Assignee | ||
Comment 7•11 years ago
|
||
Hi Ben,
Could you kindly help to assign someone to look into this issue?
Flags: needinfo?(bfrancis)
Comment 8•11 years ago
|
||
change component to browser app based on Comment 6, thanks.
Assignee: ettseng → nobody
Component: RTSP → Gaia::Browser
Comment 9•10 years ago
|
||
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)
Comment 10•10 years ago
|
||
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 | ||
Updated•10 years ago
|
Assignee: nobody → ettseng
Updated•10 years ago
|
Whiteboard: ft:system-platform
Updated•10 years ago
|
blocking-b2g: 2.1? → 2.1+
Comment 11•10 years ago
|
||
Ethan, do you have an update here?
Assignee | ||
Comment 12•10 years ago
|
||
(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.
Comment 13•10 years ago
|
||
(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).
Comment 14•10 years ago
|
||
[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
Comment 15•10 years ago
|
||
Gregor marked blocking+ but there's no comment, so requesting ni? to re-evaluate and explain the blocking reason.
Flags: needinfo?(anygregor)
Comment 17•10 years ago
|
||
Note - the dupe shows that the reduced STR is actually simple - just request to have a RTSP video be fullscreen.
Comment 18•10 years ago
|
||
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)
Updated•10 years ago
|
Flags: needinfo?(jst)
Comment 20•10 years ago
|
||
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)
Updated•10 years ago
|
Whiteboard: ft:system-platform
Assignee | ||
Comment 21•10 years ago
|
||
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
Assignee | ||
Comment 22•10 years ago
|
||
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)
Assignee | ||
Comment 23•10 years ago
|
||
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 24•10 years ago
|
||
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-
Updated•10 years ago
|
Component: DOM: Core & HTML → RTSP
Product: Core → Firefox OS
Updated•10 years ago
|
Whiteboard: [ETA:10/17]
Target Milestone: --- → 2.1 S7 (Oct24)
Assignee | ||
Comment 25•10 years ago
|
||
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. :)
Assignee | ||
Comment 26•10 years ago
|
||
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)
Assignee | ||
Comment 27•10 years ago
|
||
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 28•10 years ago
|
||
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+
Assignee | ||
Comment 29•10 years ago
|
||
(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.
Assignee | ||
Comment 30•10 years ago
|
||
Refresh comment "r=sworkman".
Attachment #8504029 -
Attachment is obsolete: true
Assignee | ||
Comment 31•10 years ago
|
||
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
Comment 32•10 years ago
|
||
Keywords: checkin-needed
Comment 33•10 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 11 years ago → 10 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 34•10 years ago
|
||
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?
Updated•10 years ago
|
Attachment #8504479 -
Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
Comment 35•10 years ago
|
||
status-b2g-v2.1:
--- → fixed
status-b2g-v2.2:
--- → fixed
status-firefox33:
--- → wontfix
status-firefox34:
--- → wontfix
status-firefox35:
--- → fixed
Whiteboard: [ETA:10/17]
Comment 36•10 years ago
|
||
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.
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Comment 37•10 years ago
|
||
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
Updated•10 years ago
|
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.
Description
•