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.

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?

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.

It's a protocol error on PNecko::Msg_PHttpChannelConstructor ...

Aha, bug 782542 relanded.

Root cause of regression is known now - removing regressionwindow-wanted.

Jason or Josh should be able to help out.

I/Gecko ( 840): NeckoParent::AllocPHttpChannel: FATAL error: missing required PBrowser argument: KILLING CHILD PROCESS

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.

For those following along at home, http://mxr.mozilla.org/mozilla-central/source/b2g/components/YoutubeProtocolHandler.js#44 is create a context-less XHR. :(

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.

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.

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 :/

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.

I'm working on a solution that moves the XHR to the chrome process.

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.

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

Have at it, David.

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.

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

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

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.

I'll implement comment 26.

Re-adding the whiteboard label we need for internal tracking purposes.

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.

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 on attachment 698461 [details] [diff] [review] Part 2: Gaia flagging myself for feedback so I don't forget this bug.

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.

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

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.

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.

Josh, I'm not able to build gecko, so I'll take it back. Thanks for your help!

oops, I meant "I'm now able" not "I'm not able".

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.

Here's the gecko side of the patch

Having firmly established how inexperienced I am with gaia, I think someone else should review this.

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.

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.

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

Comment on attachment 698818 [details] link to patch on github Merged in: https://github.com/mozilla-b2g/gaia/commit/30eb2e942cbb5861131ccb7194274d1e4cad28a0

Gaia part has merged, so can be resolved when it hits central

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?