1003653 - [e10s] "YouTube High Definition" addon does not automatically change YouTube video settings

Status: VERIFIED FIXED

(Firefox :: Extension Compatibility, defect)

Description

[Chris Peterson [:cpeterson]]

The "YouTube High Definition" addon is a featured addon on AMO's home page.

STR:
1. In a non-e10s window, install "YouTube High Definition" addon: https://addons.mozilla.org/en-US/firefox/addon/youtube-high-definition/
2. Restart Firefox in e10s mode.
3. Watch console.

RESULT:

1398829172688	addons.manager	WARN	Exception calling callback: TypeError: addon is null (chrome://youtubehighdefinition/content/youtubehighdefinition.js:864:4) JS Stack trace: YoutubeHighDefinition.checkFlashBlock/<@youtubehighdefinition.js:864:5 < [e-mail]:166:5 < [e-mail]:1942:9 < [e-mail]:261:7 < [e-mail]:1937:13 < [e-mail]:120:7 < [e-mail]:192:5 < [e-mail]:1933:1 < [e-mail]:267:7 < [e-mail]:1937:13 < [e-mail]:363:7 < [e-mail]:192:5 < [e-mail]:1933:1 < [e-mail]:267:7 < [e-mail]:1937:13 < [e-mail]:3624:7 < makeSafe/<@XPIProviderUtils.js:146:17 < [e-mail]:127:5 < this.XPIDatabase.getAddon/<@XPIProviderUtils.js:1091:9 < [e-mail]:863:11 < [e-mail]:742:7

JavaScript strict warning: chrome://youtubehighdefinition/content/youtubehighdefinition.js, line 57: reference to undefined property Components.classes['@mozilla.org/extensions/manager;1']

JavaScript strict warning: chrome://youtubehighdefinition/content/youtubehighdefinition.js, line 577: assignment to undeclared variable mi

Comment 1

[Jorge Villalobos [:jorgev] (he/him)]

Looking at the source, the error seems to be a fairly harmless bug:

> AddonManager.getAddonByID("{3d7eb24f-2740-49df-8937-200b1cc08f8a}", function(addon) {  
> 	YoutubeHighDefinition.flashBlockInstalled=addon.isActive;
> });

It should be checking for 'addon' being not null, so the code doesn't fail. The same happens on 29.

CC'ing add-on dev.

Comment 2

[Baris Derin]

Attachment: Fix for addon bug

Comment 3

[Baris Derin]

Thanks Chris for the bug report. Could you try attached XPI and let me know if it is OK?

@Jorge, thanks for the tip. Is this urgent? Do you want me to upload this fix to AMO?

Comment 5

[Chris Peterson [:cpeterson]]

Thanks, Baris! I think there are two unrelated bugs here:

1. The "TypeError: addon is null" JS error. I verified that your attached XPI does fix that error, but that error was not related to e10s mode. It affects any Firefox user who does not have Flashblock installed. This issue is not urgent because it does not break your addon's functionality.

2. YouTube High Definition settings are not automatically applied to a YouTube video when running in Firefox's e10s sandbox. This problem only affects users testing e10s, so this is not an urgent issue. To test e10s, you just need to install Nightly and then use the "File > Open e10s Window" menu to open a new e10s sandbox window.

When I load a YouTube video in an e10s window, the video plays using YouTube's default settings, not the YouTube High Definition settings. I don't see any obvious error messages in the Browser Console. However, if I change YouTube High Definition settings using the addon's toolbar button menu while watching the video, then the new video settings *are* applied correctly! :) So the bug is related to the addon (or Firefox?) failing to *automatically* apply the YouTube High Definition settings when a video starts playing.

Comment 6

[Baris Derin]

Thanks for clarification Chris. 

Adding event listeners to gBrowser does not seem to work for e10s. Add-on needs to detect loaded web pages by adding a DOMContentLoaded event listener to gBrowser object which is detailed here.

https://developer.mozilla.org/en-US/Add-ons/Code_snippets/Tabbed_browser#Detecting_page_load

It is in youtubehighdefinition.js, line 83
gBrowser.addEventListener("DOMContentLoaded",YoutubeHighDefinition.aboutblanklistener,true)

After entering e10s, I tried to load YouTube and other web pages to trigger the added event listener but it did not help. But strangely, the same added event listener triggers for Chrome pages like about:newtab and about:config in e10s mode. 

Because add-on works for Normal mode and event listener is triggered for only e10s Chrome pages, there should be something preventing event listeners to trigger for Web pages in e10s mode.

Comment 7

[Chris Peterson [:cpeterson]]

(In reply to Baris Derin from comment #6)
> Adding event listeners to gBrowser does not seem to work for e10s. Add-on
> needs to detect loaded web pages by adding a DOMContentLoaded event listener
> to gBrowser object which is detailed here.

We are still revising our MDN documentation about e10s, but the following blog posts have some good code examples:

http://billmccloskey.wordpress.com/2013/12/05/multiprocess-firefox/

http://timtaubert.de/blog/2011/08/firefox-electrolysis-101/

As described in the blog posts, I think you will need to use global message manager to load a content script in the content/child process that listens for DOMContentLoaded. gBrowser runs in the chrome/parent process, so it will see events for chrome pages like about:newtab and about:config, but not normal web content.

When the user changes their YouTube High Definition preferences (in the chrome process), you could use sendAsyncMessage() to broadcast the new video settings to your content script in the content process. When your content script detects that a YouTube page has loaded, it would already have the user's video settings and can then modify the YouTube page right away.

Alternately, your content script could use sendAsyncMessage() to ask the chrome process for the user's video settings when a YouTube page is loaded, but that sounds more complicated (and slower) than just broadcasting the video setting changes from the chrome process to the content script. Users rarely change their video settings, but load pages all the time.

Please feel free to keep asking e10s questions in this bug, but you can also drop by the #e10s IRC channel (on irc.mozilla.org) for faster responses. :)

Comment 8

[Baris Derin]

Attachment: e10s.xpi

Comment 9

[Baris Derin]

(In reply to Chris Peterson (:cpeterson) from comment #7)

Thanks again Chris for great clarification. I attached the XPI for e10s support. Would you mind looking into how I tried to solve the problem and recommend if possible?

+ Add-on loads frame scripts via Component level to prevent multiple script loads.
+ I load the same overlay.js file as content script for e10s mode which I use for normal Firefox mode already (e.g for Firefox 29).

+ On youtubehighdefinition.js line 937, I set the e10s property as "true" assuming that all Firefox 
UI windows are e10s mode window for testing purposes. Because I could not find a way how to detect a Firefox window running in e10s mode. Is there any property or method that you can recommend to detect e10s mode Windows?

+ On youtubehighdefinition.js line 938, I check for "content" object. If it is available then the script is running as child process content script and add-on adds event listeners and message listeners for child process. If "content" is not available (assuming that in normal mode there is no window.content at the time overlay.js is inserted into main Firefox window) then the script is run as overlay.js for normal mode Window and the event and message listeners are not added. 

+ On youtubehighdefinition.js line 950, checking for FlashBlock but because AddonManager is not available for child process content script, it throws error. I will handle this by message listener later.

+ On youtubehighdefinition.js line 643, I am again assuming we are in e10s mode for testing purposes and handling Option changes from Toolbar Button by message listeners. I could not use "content" here to differentiate scripts because Toolbar button functionality use overlay.js.

In short, I need a way to determine a windows is e10s or not. Also I need your thoughts on using "content" object to differentiate normal and e10s mode?

Thanks

Comment 10

[Chris Peterson [:cpeterson]]

Baris: I just tested your new e10s.xpi (in e10s mode and normal, non-e10s mode) and it seems to work! :D

AFAIK, addons should not need to differentiate between normal and e10s mode. The content scripts and message managers should work exactly the same in both modes. I CC'd billm who would know. Or you can ask someone on #e10s.

Unfortunately, Firefox has its own e10s bugs related to Flash and YouTube, so it was hard to know if your new addon works in all cases. Some issues I saw:

* When changing the resolution of HTML5 video, your version 8.1 addon displays a warning saying "You are using YouTube in HTML5 Mode. In HTML5 mode you can only select the Highest Resolution which is automatically determined by YouTube as 720p." Your new addon version 8.2 does not display this warning.

* When playing Flash video with full-page mode, the video player is correctly resized to fit the page, but it looks like the video player was just scaled up, not redrawn. The controls are too big and pixelated. I suspect this is a known Firefox bug with e10s Flash.

Comment 11

[Baris Derin]

Attachment: e10s_2.xpi

Comment 12

[Baris Derin]

Chris, thanks again. 

I see. I fixed HTML5 warning with new XPI. Could not produce other Flash problem on my side. It might be related Hardware Acceleration for Flash. As default YouTube videos only play Hardware Accelerated on Full Screen.

Biggest concern for me is to determine the very window is e10s or normal mode.

Comment 13

[Chris Peterson [:cpeterson]]

(In reply to Baris Derin from comment #12)
> Biggest concern for me is to determine the very window is e10s or normal
> mode.

So you want to know whether the window is e10s or normal so you can avoid calling nsiPromptService from your content script? Can you send a message from the content process to the chrome process and let the chrome process call nsiPromptService instead? Code that is written using the message manager and content scripts should work in both e10s and non-e10s windows, so you then won't need to know whether your script is running in an e10s window.

Comment 14

[Baris Derin]

Attachment: e10s_3.xpi

Comment 15

[Baris Derin]

(In reply to Chris Peterson (:cpeterson) from comment #13)

OK, finally found it digging inside the Firefox source code. It is 

gMultiProcessBrowser

property that can differentiate e10s and non-e10s modes.

Thanks again for your explanation. I just wanted to make sure that it does not load content script for Firefox 29 unnecessarily.

Comment 16

[Chris Peterson [:cpeterson]]

I tested your e10s_3.xpi and it correctly resizes YouTube Flash and HTML5 videos.

I can't verify that it works 100% with Flash videos because I have unrelated Firefox problems playing Flash videos with e10s. I'm testing OS X, but it sounds like resizing Flash works for you. I'll mark this bug as fixed after you publish the next version of your addon to AMO.

Comment 17

[Baris Derin]

Chris, I uploaded next version to AMO. Thanks again for your all help.
https://addons.mozilla.org/en-US/firefox/addon/youtube-high-definition/versions/8.2

Comment 18

[Chris Peterson [:cpeterson]]

Version 8.2 works correctly for me in e10s mode. :)

Comment 19

[Chris Peterson [:cpeterson]]

I verified that YouTube High Definition's "Video Size" feature works correctly in Nightly 34. Note that YouTube has started serving HTML5 video instead of Flash video to Firefox Nightly users, so YouTube High Definition's "Video Quality" feature can't be tested (because AFAIK it only works for Flash videos).