Closed
Bug 825698
Opened 12 years ago
Closed 12 years ago
Youtube videos loaded in browser not being sent to video app at all
Categories
(Firefox OS Graveyard :: General, defect, P1)
Tracking
(blocking-basecamp:+, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18 fixed)
People
(Reporter: cjones, Assigned: djf)
References
Details
(Keywords: regression, smoketest, Whiteboard: [cr 436772])
Attachments
(3 files, 5 obsolete files)
5.95 KB,
patch
|
jduell.mcbugs
:
review+
|
Details | Diff | Splinter Review |
108 bytes,
text/html
|
daleharvey
:
review+
|
Details |
6.84 KB,
patch
|
jdm
:
review+
|
Details | Diff | Splinter Review |
STR
(1) Load youtube in browser
(2) Try to play a video
We immediately get the frowny face.
Logcat nearby is
I/IdleService( 104): next timeout 1000 msec from now
I/IdleService( 104): SetTimerExpiryIfBefore: next timeout 1000 msec from now
I/IdleService( 104): reset timer expiry to 1009 msec from now
I/IdleService( 104): Reset idle timeout: tell observer 480fe3d0 user is back
D/memalloc( 104): /dev/pmem: Freeing buffer base:0x49ea3000 size:245760 offset:299008 fd:171
D/memalloc( 104): /dev/pmem: Freeing buffer base:0x49f84000 size:245760 offset:1220608 fd:186
I/Gecko ( 104): [Parent 104] WARNING: waitpid failed pid:705 errno:10: file /home/cjones/mozilla/new-b2g/gecko/ipc/chromium/src/base/process_util_posix.cc, line 260
which often suggests that gecko intentionally killed the content process.
Isn't this one of the smoketests? :(
Broken feature, needs to block.
Reporter | ||
Comment 1•12 years ago
|
||
Gecko is killing it
Program received signal SIGTERM, Terminated.
write () at bionic/libc/arch-arm/syscalls/write.S:10
djf, did you see this while working on the video app recently?
Comment 2•12 years ago
|
||
So apparently the fix in bug 821514 didn't work then.
Right now, it is not a smoketest actually, but it's the larger suite of tests ran when we do an acceptance test run.
Reporter | ||
Comment 3•12 years ago
|
||
It's a protocol error on PNecko::Msg_PHttpChannelConstructor ...
Comment 5•12 years ago
|
||
Root cause of regression is known now - removing regressionwindow-wanted.
Keywords: regressionwindow-wanted
Jason or Josh should be able to help out.
Reporter | ||
Comment 7•12 years ago
|
||
Attachment #696820 -
Flags: review?(jduell.mcbugs)
Reporter | ||
Comment 8•12 years ago
|
||
I/Gecko ( 840): NeckoParent::AllocPHttpChannel: FATAL error: missing required PBrowser argument: KILLING CHILD PROCESS
Reporter | ||
Comment 9•12 years ago
|
||
Not immediately obvious again how we can claw our way out to a context here, but we should be able to find one somehow.
Maybe too late now, but we should written the parent-side checks as the union of capabilities of the TabParents in a given ContentParent. That is, if any of the active TabParents in a ContentParent can do the network load, then allow it.
Comment 10•12 years ago
|
||
For those following along at home, http://mxr.mozilla.org/mozilla-central/source/b2g/components/YoutubeProtocolHandler.js#44 is create a context-less XHR. :(
Comment 11•12 years ago
|
||
The suggestion in comment 9 might be the quickest way forward; I'm at a loss as to how to give the XHR some context given the... unorthodox manner in which it's used. I may be able to move forward on that tomorrow.
Reporter | ||
Comment 12•12 years ago
|
||
Even better would be, if the load request doesn't include a TabParent, warn/NS_ASSERTION and fall back to checking the union of capabilities of all TabParents.
Will comment 9 help? Does any TabParent ever have the right to do a context-free load?
At the time when newChannel is called we unfortunately don't have any way of getting to a context which we can use. This is just one of the ways that the Necko API sucks.
But at the time when AsyncOpen is called on the channel, the channel should have a loadgroup which means that we should be able to dig out a context.
Comment 14•12 years ago
|
||
The YoutubeProtocolHandler doesn't work the way other protocol handlers do, in that newChannel doesn't actually return a channel. It initiates an XHR to obtain relevant data, then throws an error code while the real work happens inside the XHR's load handler, which sends data to the parent process :/
Comment 15•12 years ago
|
||
Jonas is right, however; in my enthusiasm for Chris' suggestion I forgot that the Necko security checks aren't capability-based. We just obtain an app ID and browser status from the passed actor, and throw up our hands if none is available. If there's some way to get a valid app ID and browser status from a ContentParent, I can work from there, otherwise we're going to have to figure out some way to salvage the youtube protocol handler.
Comment 16•12 years ago
|
||
I'm working on a solution that moves the XHR to the chrome process.
Assignee: nobody → josh
Assignee | ||
Comment 17•12 years ago
|
||
Another solution might be to make the Video app activity handler youtube-specific and move the XHR code into Gaia. This assumes that there is a permission we can set to allow the video app to do cross-origin XHR, of course. Feel free to assign the bug to me if you'd like me to do it that way.
Updated•12 years ago
|
blocking-basecamp: ? → +
Reporter | ||
Comment 18•12 years ago
|
||
(In reply to David Flanagan [:djf] from comment #17)
> Another solution might be to make the Video app activity handler
> youtube-specific and move the XHR code into Gaia. This assumes that there
> is a permission we can set to allow the video app to do cross-origin XHR, of
> course. Feel free to assign the bug to me if you'd like me to do it that way.
I much prefer this approach.
Comment 20•12 years ago
|
||
That being said, I just tested a patch which implemented comment 16 and fixed the problem here, so there is a backup plan if everything goes to pot.
Comment 21•12 years ago
|
||
Comment 22•12 years ago
|
||
Attachment #697138 -
Attachment is obsolete: true
Updated•12 years ago
|
Attachment #696820 -
Flags: review?(jduell.mcbugs) → review+
Comment 23•12 years ago
|
||
Just FYI to answer sicking's questions in comment 13
> Does any TabParent ever have the right to do a context-free load?
Not unless we do a security audit to make sure it's not a security risk. No context maps to "I get to read/write system cookies, etc"...
> At the time when newChannel is called we unfortunately don't have any way of
> getting to a context which we can use.
Security info is set via SetNotificationCallbacks, and can be called at any time until asyncOpen, so you don't need to set it in newChannel.
> This is just one of the ways that the Necko API sucks.
liar liar PoF :)
Reporter | ||
Comment 24•12 years ago
|
||
Whiteboard: [leave open]
Assignee | ||
Comment 25•12 years ago
|
||
I'm trying to transfer the youtube handler code from gecko to gaia, but am stymied by my inabilty to build a version of gecko that will boot on my phone...
Updated•12 years ago
|
Target Milestone: --- → B2G C4 (2jan on)
Assignee | ||
Comment 26•12 years ago
|
||
My plan for this bug is to modify YouTubeProtocolHandler.js to have this newChannel function:
newChannel: function yt_phNewChannel(aURI) {
/*
* This isn't a real protocol handler. Instead of creating a channel
* we just send a message and throw an exception. This 'content-handler'
* message is handled in b2g/chrome/content/shell.js where it starts
* an activity request that will open the Video app. The video app
* includes code to handle this fake 'video/youtube' mime type
*/
cpmm.sendAsyncMessage("content-handler", {
type: 'video/youtube',
url: aURI
});
throw Components.results.NS_ERROR_ILLEGAL_VALUE;
},
Then I'll move all the youtube-specific stuff to gaia/apps/video/js/youtube.js and handle it all in Gaia.
But because of bug 825840 and the fact that I have a mac, I cannot build gecko to make and test this change myself.
If anyone wants to take the bug, feel free. If mac builds don't get fixed soon, I'll unassign myself.
Comment 27•12 years ago
|
||
I'll implement comment 26.
Assignee: dflanagan → josh
Whiteboard: [leave open] [cr 436772] → [leave open]
Comment 28•12 years ago
|
||
Re-adding the whiteboard label we need for internal tracking purposes.
Whiteboard: [leave open] → [leave open] [cr 436772]
Comment 29•12 years ago
|
||
Assignee | ||
Comment 30•12 years ago
|
||
Josh,
The bug that was preventing me from building gecko is apparently resolved so I can take this back if you would like.
And if not, I'd be happy to review your patch.
Comment 31•12 years ago
|
||
That would be convenient. I've got a patch, but I can't figure out how to debug why it's not working, since I can't find console output anywhere.
Comment 32•12 years ago
|
||
Comment 33•12 years ago
|
||
Assignee | ||
Comment 34•12 years ago
|
||
Comment on attachment 698461 [details] [diff] [review]
Part 2: Gaia
flagging myself for feedback so I don't forget this bug.
Attachment #698461 -
Flags: feedback?(dflanagan)
Assignee | ||
Comment 35•12 years ago
|
||
Josh,
First, you should change this chrome code:
+ var xhr = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"]
+ .createInstance(Ci.nsIXMLHttpRequest);
To the equivalent content code:
var xhr = new XMLHttpRequest()
Is console.log() not producing output that you can see with adb logcat | grep Console? That's what I always do.
Assignee | ||
Comment 36•12 years ago
|
||
Comment on attachment 698461 [details] [diff] [review]
Part 2: Gaia
Review of attachment 698461 [details] [diff] [review]:
-----------------------------------------------------------------
I tried to test this, but built gecko from the b2g18 tree, and it turns out that bug 825840 isn't fixed there, yet, only on m-c.
I don't have time to build a patched version of m-c tonight, so I'll just offer these comments without being able to test it.
I will attach the modified version of the gaia patch that I was going to test with.
::: apps/video/js/view.js
@@ +16,5 @@
> + return;
> + }
> +
> + var xhr = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"]
> + .createInstance(Ci.nsIXMLHttpRequest);
new XMLHttpRequest() in content code.
@@ +17,5 @@
> + }
> +
> + var xhr = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"]
> + .createInstance(Ci.nsIXMLHttpRequest);
> + xhr.open("GET", data.url, true);
If it was me, I'd pass the completely raw url from gecko to this point, and then extract the video id and create the get_video_info URL here in Gaia. That way, if youtube changes their query API we just need to update the app, not gecko.
@@ +18,5 @@
> +
> + var xhr = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"]
> + .createInstance(Ci.nsIXMLHttpRequest);
> + xhr.open("GET", data.url, true);
> + xhr.addEventListener("load", function() {
We should have an "error" handler here, for HTTP errors. I don't know if we can report those errors to the user meaningfully (because I doubt we can localize all HTTP error codes), but we can at least use console.error() to get them into logcat.
@@ +29,5 @@
> + // throws an Error. We report the error message here and do
> + // nothing. This shouldn't happen often. But if it does, the user
> + // will find that clicking on a video doesn't do anything.
> + var message = e.message.join ? e.message.join(" ") : e.message;
> + handleError(activity, message);
You don't need to do the join() stuff here. That was in the log() function that I copied from some other chrome module, but isn't actually needed for the errors we throw.
handleError() isn't really designed for errors thrown by parseYoutubeVideoInfo() however. We could think about internationalizing those errors, but I don't know if it is worth it.
When this code was in chrome, errors thrown here would be logged, and then nothing would happen. The user would find that tapping on the link or video didn't do anything.
Here, the user has tapped on the video and the viewer has opened, so I guess it wouldn't be good to just close at this point...
Maybe calling handleError() is the best we can do here.
@@ +33,5 @@
> + handleError(activity, message);
> + }
> + });
> + xhr.send(null);
> + displayVideo(activity, data);
This line shouldn't be here, I think.
@@ +70,5 @@
> + // Hopefully this error message will be properly localized.
> + // Do we need to add any parameters to the XMLHttpRequest to
> + // specify the language we want?
> + //
> + // Note that we include fake type and url properties in the returned
There are a bunch of trailing spaces in the lines above. fixjsstyle will remove them for you
@@ +92,5 @@
> +
> + // Now parse the available streams
> + var streamsText = params.url_encoded_fmt_stream_map;
> + if (!streamsText)
> + throw Error("No url_encoded_fmt_stream_map parameter");
This message is not one we want to display to the user with handleError(), even if it could be localized. Its not likely to ever happen, though.
@@ +121,5 @@
> + var bestStream = streams[0];
> +
> + // If the best stream is a format we don't support just return
> + if (formats.indexOf(bestStream.itag) === -1)
> + throw Error("No supported video formats");
This error message is useful, but probably not worth doing late l10n to localize it.
How about when this function throws an error we call handleError() with no message. Then I think the user will see the localized message "YouTube won't play video". Its not quite accurate, but seems good enough given our timeline now.
@@ +144,5 @@
> + return result;
> + }
> +});
> +
> +function handleError(activity, message) {
Can't this function just go inside the parenthesis above? If so, you don't need to add the activity argument to it.
Note also that the code below in displayVideo() calls handleError with just the message argument.
@@ +155,5 @@
> + var prefix = navigator.mozL10n.get('youtube-error-prefix');
> +
> + // Display the error message to the user
> + // XXX Using alert() is simple but ugly.
> + alert(prefix + '\n\n' + message);
If we're going to pass null messages, take out the newlines and be careful how you do the concatenation so you don't convert null or undefined to "null" or "undefined".
Assignee | ||
Comment 37•12 years ago
|
||
Here's modified version of your gaia patch with some of my feedback comments incorporated. This is the version I was going to test with before I discovered that my gecko build was still broken by bug 825840.
Comment 38•12 years ago
|
||
Continue to feel free to take this over. I've tried logcat and can't see any output being generated by a console.log call in the video activity handler, so I haven't been able to make any progress.
Assignee | ||
Comment 39•12 years ago
|
||
Josh,
I'm not able to build gecko, so I'll take it back. Thanks for your help!
Assignee: josh → dflanagan
Assignee | ||
Comment 40•12 years ago
|
||
oops, I meant "I'm now able" not "I'm not able".
Assignee | ||
Comment 41•12 years ago
|
||
Josh,
Here's my gaia patch. I put all the youtube-specific stuff in a separate youtube.js file. Note that I'm also passing the full original vnd.youtube:///id url from chrome, so the parsing code for that is in gaia now too.
I realize now that the problem you were having was probably that the activity was never being launched because you hadn't changed video/manifest.webapp to add the 'video/youtube' type and grant systemXHR permission.
Let me know if you'd prefer that I found someone else to review this.
Attachment #698461 -
Attachment is obsolete: true
Attachment #698544 -
Attachment is obsolete: true
Attachment #698461 -
Flags: feedback?(dflanagan)
Attachment #698818 -
Flags: review?(josh)
Assignee | ||
Comment 42•12 years ago
|
||
Here's the gecko side of the patch
Attachment #697139 -
Attachment is obsolete: true
Attachment #698460 -
Attachment is obsolete: true
Attachment #698820 -
Flags: review?(josh)
Updated•12 years ago
|
Attachment #698820 -
Flags: review?(josh) → review+
Comment 43•12 years ago
|
||
Having firmly established how inexperienced I am with gaia, I think someone else should review this.
Assignee | ||
Comment 44•12 years ago
|
||
Comment on attachment 698818 [details]
link to patch on github
Dale,
This is the youtube patch you reviewed not long ago, but this time all of the youtube query code has moved from chrome to content. This turned out to be necessary because there were security problems using XHR in that particular chrome. I've also added a spinner animation while youtube is being queried.
Attachment #698818 -
Flags: review?(josh) → review?(dale)
Assignee | ||
Comment 45•12 years ago
|
||
Setting checkin-needed for https://bugzilla.mozilla.org/attachment.cgi?id=698820
That patch needs to go to m-c and into b2g18. I don't know about aurora.
This patch only affects b2g/components/YoutubeProtocolHandler.js
The existing handler is completely broken, so landing this before the gaia part has landed won't make anything worse than it already is.
Keywords: checkin-needed
Comment 46•12 years ago
|
||
Comment on attachment 696820 [details] [diff] [review]
Log process-kill messages in all builds
landed the printf_stderr patch on b2g18:
https://hg.mozilla.org/releases/mozilla-b2g18/rev/17558806e0fc
Updated•12 years ago
|
Priority: -- → P1
Comment 47•12 years ago
|
||
Keywords: checkin-needed
Comment 48•12 years ago
|
||
status-b2g18:
--- → fixed
status-firefox19:
--- → wontfix
status-firefox20:
--- → wontfix
status-firefox21:
--- → fixed
Comment 49•12 years ago
|
||
Comment on attachment 698818 [details]
link to patch on github
Merged in: https://github.com/mozilla-b2g/gaia/commit/30eb2e942cbb5861131ccb7194274d1e4cad28a0
Attachment #698818 -
Flags: review?(dale) → review+
Comment 50•12 years ago
|
||
Gaia part has merged, so can be resolved when it hits central
Whiteboard: [leave open] [cr 436772] → [cr 436772]
Comment 51•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 53•10 years ago
|
||
Do I understand correctly?
When you try to play a Youtube video in the Browser app, the Video Player app should open with the Youtube video?
You need to log in
before you can comment on or make changes to this bug.
Description
•