930788 - [e10s] Video DownloadHelper doesn't work with e10s

Status: RESOLVED WORKSFORME

(Firefox :: Extension Compatibility, defect)

Description

[Chris Peterson [:cpeterson]]

https://addons.mozilla.org/en-US/firefox/addon/video-downloadhelper/

Comment 1

[Chris Peterson [:cpeterson]]

Video DownloadHelper can download Flash videos from Vimeo and HTML5 (webm) videos from YouTube, but it does not recognize Flash videos on YouTube so Video DownloadHelper's list of downloadable videos is empty.

Comment 2

[Tom S [:evilpie]]

Attachment: some fixes

With this diff for: "video_downloadhelper-4.9.21-fx+sm.xpi" I can get the addon working on youtube. The other big module/probe "NetProbe" would require us to fire "http-on-modify-request" in the child.

Comment 3

[Chris Peterson [:cpeterson]]

I can reproduce this bug with Aurora 32, but I can't test Nightly 33 because YouTube is only serving HTML5 video to Nightly: bug 1040972.

Comment 4

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

e10s isn't enabled by default yet even in Nightly, so clearing the status

Comment 5

[Allison Naaktgeboren :ally]

cpeterson, is this still re-producing?

Comment 6

[Chris Peterson [:cpeterson]]

Yes. I can reproduce this bug on DailyMotion. YouTube and Vimeo now serve HTML video instead of Flash video for Firefox 34 and later.

http://www.dailymotion.com/video/x18opnp_lil-bub-magical-yule-log-video_animals

Comment 7

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

The developer has been contacted through AMO.

Comment 8

[Michel Gutierrez]

There is an effort going on rewriting entirely Video DownloadHelper, making it restartless and using the new addon structure. Given the issue with e10s, it looks like i need to push further on this.

If i understand well, e10s has just been introduced into nightly. When will it make it to Aurora ?

Comment 9

[Chris Peterson [:cpeterson]]

(In reply to Michel Gutierrez from comment #8)
> If i understand well, e10s has just been introduced into nightly. When will
> it make it to Aurora ?

Based on our bug fix rate, e10s will likely ride from Nightly to Aurora 39 for Firefox's 2015-02-23 merge date.

Comment 10

[Allison Naaktgeboren :ally]

Telemetry is reporting that VDH is regularly throwing a js exception in util-service.jsm line 322.

Comment 11

[Michel Gutierrez]

VDH is currently being entirely rewritten, this exception is very likely to vanish in the next version.
Let me know if it is very important to fix the issue in the current version.

Comment 12

[spaceshipgalore]

v5.0.0 alpha 1 has been released by the developer: https://groups.google.com/forum/#!forum/video-downloadhelper-5

This uses the firefox addon sdk instead of XUL.

Comment 13

[Michel Gutierrez]

Yes, we launched a first alpha version yesterday. So far, the feedback is rather good.

It has not been tested against e10s yet, but since it is sdk based, if it does not work right away, this shouldn't be too hard to fix.

I would like to change the title of this bug as it has nothing to do with the actual problem, but i cannot find how to do so from the bugzilla UI. Maybe it requires privileges i do not have. Can someone do it for me, like "Video DownloadHelper is not e10s compliant" ?

Comment 14

[Michel Gutierrez]

Blocked by bug #1135002 (blocking is not editable by myself on bugzilla UI)

DownloadHelper relies on the ability to inject content code into an arbitrary windows object, discovered by monitoring http channels. The only way i found to do so is by using the sdk/content/content-worker.js which is not found since Fx38 even if present in the source code.

Comment 15

[Michel Gutierrez]

My latest development code seems to work (basic tests only) on the last nightly and aurora. Can i assume it is e10s compliant ?

Comment 16

[Chris Peterson [:cpeterson]]

Michel, if your development code works on your machine, then you are probably good to go. :) You should also check Firefox's Browser Console for any warnings or errors that might be reported.

Once your latest code is upon on AMO, we can mark this bug fixed. We like to keep the bug reports open until the fixed extension is on AMO so normal users don't get confused when their extension from AMO is broken but the bug is marked fixed.

Comment 17

[Michel Gutierrez]

Great ! I didn't want the bug to be closed before being pushed to amo, just wanted to make sure it was not something like "you need to enable this in the config to truly use e10s". So it's good news :)

Comment 18

[Michel Gutierrez]

Humm, it was working on nightly because e10s was disabled. With e10s enabled, i'm stuck with an issue for which i cannot find a solution in the documentation nor with experiments.

Normally (no e10s), VDH monitors HTTP channels and upon finding a matching request/response, it does something like this:

var window = channel.notificationCallbacks.QueryInterface(Ci.nsIInterfaceRequestor)
    .getInterface(Ci.nsIDOMWindow);

This window object is then used to establish safe communications between the chrome part of the addon and the content window that originated the HTTP request:

var worker = Worker({
    window: window,
    ....
});

When e10s is enabled, trying to get the window from the channel, i get an exception:

Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsIInterfaceRequestor.getInterface]

The channel holds a loadInfo.windowInnerId value that looks promising but i don't see a way to create a Worker from this id.

Any suggestion ?

Comment 19

[Chris Peterson [:cpeterson]]

Jim, do you have any suggestions for how the Video DownloadHelper add-on can monitor HTTP channels in e10s?

Comment 20

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Starting in FF38 you can get the <browser> element from the channel using notificationCallbacks to get an nsILoadContext and then accessing topFrameElement on that.

However, I'm not sure what a Worker is. It sounds like it's part of the add-on SDK. Dave, do you know how we could fix this?

Comment 21

[Michel Gutierrez]

Thanks, that looks like a lead to follow. However, DownloadHelper needs to get the actual frame that generated the HTTP request. It's often the top frame but not always.

A content worker is an add-on SDK object to establish safe 2-ways messaging between the chrome part of the add-on and content.

Comment 22

[Dave Townsend [:mossop]]

Yeah so right now Worker accepts a CPOW window to attach to. Once we've updated all the SDK APIs we'll be deprecating that option and instead you'll have to either do something in the content process or provide a window ID to attach to. I'm going to be writing up some docs and example code for bug 1130529 today, this might make for a good case.

Comment 23

[Michel Gutierrez]

As an update, VDH 5.0.1 is in the review queue at amo but this version is still not e10s compliant.

Once the documented API for attaching workers from a window ID is ready, i'll make the changes in the add-on.

Comment 24

[Giorgio Maone [:ma1]]

Taking ownership of this bug and working more closely with Michel to help fixing the remaining issues.

Comment 25

[Michel Gutierrez]

That's excellent news ! Thanks Giorgio.

Comment 26

[Giorgio Maone [:ma1]]

Michel has just published an e10s-compatible RC,

https://groups.google.com/forum/#!topic/video-downloadhelper-5/PWMMdAxFOew

(warning: uglified sources).

Instead of using loadContext.topFrameElement.messageManager and asking a frame script to perform the title-extracting DOM manipulation, as I originally suggested, it takes the "shortcut" of grabbing loadContext.topFrameElement._contentWindow and attaching a content working to it, which I suspect is not very future-proof.

However it does work for now, so I'm tentatively resolving this bug as fixed.

Feel free to reopen as soon as this gets deprecated/broken, and we'll work together to implement a long-term solution, based either on frame scripts or on Bug 1130529's SDK API, if better suitable and documented.

Comment 27

[Michel Gutierrez]

Well, it is certainly too early to say that all Video DownloadHelper problems related to e10s have been found and fixed, but at least the blocking point with accessing page content from an HTTP channel seems to be working.

About using frame scripts versus add-on SDK method to access content, i prefer being consistent with my add-on code, where add-on SDK workers are being used at two dozens other places.

Note that i had to use a frame script somewhere in the add-on to execute some code that could not run within a content script (using Canvas2DContext.drawWindow() to take screenshots of the content).

Thanks Giorgio for pointing me to the right documentation.

Comment 28

[Michel Gutierrez]

Version 5.2.0 with e10s fix submitted to amo.

Comment 29

[Gabor Krizsanits (INACTIVE)]

Assignee: Gabor Krizsanits
Link to add-on: https://addons.mozilla.org/firefox/downloads/latest/3006/addon-3006-latest.xpi?src=dp-btn-primary
Add-on ID: {ec8030f7-c20a-464f-9b0e-13a3a9e97384}
How well does it work?: 99% (it worked great for me, Michel mentioned some e10s related bugs earlier which I don't know if still exists)
Steps to reproduce working features:
- downloading videos in different resolution from youtube works, other sites I tested worked too
- options menu GUI seem to work just fine
- capturing video of the browser (after installing a helper the add-on requires) worked fine too

Steps to reproduce broken features:
- I could not find any

Any obvious performance problems? no, it's working smoothly

Chromium version: It is not supported. Even simpler add-ons trying to do something similar like flv_download were kicked out from the Chrome Extension Gallery.

Comment 30

[Gabor Krizsanits (INACTIVE)]

(In reply to Dave Townsend [:mossop] from comment #22)
> Yeah so right now Worker accepts a CPOW window to attach to. Once we've
> updated all the SDK APIs we'll be deprecating that option and instead you'll
> have to either do something in the content process or provide a window ID to
> attach to. I'm going to be writing up some docs and example code for bug
> 1130529 today, this might make for a good case.

This happened already, Worker doesn't accept CPOW windows any more, which means the add-on is broken right now on nightly. What is the easiest way to fix this Dave? Can we expose a remote version of the Worker somehow?

Comment 31

[Dave Townsend [:mossop]]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #30)
> (In reply to Dave Townsend [:mossop] from comment #22)
> > Yeah so right now Worker accepts a CPOW window to attach to. Once we've
> > updated all the SDK APIs we'll be deprecating that option and instead you'll
> > have to either do something in the content process or provide a window ID to
> > attach to. I'm going to be writing up some docs and example code for bug
> > 1130529 today, this might make for a good case.
> 
> This happened already, Worker doesn't accept CPOW windows any more, which
> means the add-on is broken right now on nightly. What is the easiest way to
> fix this Dave? Can we expose a remote version of the Worker somehow?

sdk/deprecated/sync-worker is basically the old worker that used CPOWs rather than e10s support so you could switch to that.

Comment 32

[Gabor Krizsanits (INACTIVE)]

Dave fixed the CPOW support for worker, so this should be a non issue again. (bug 1146926)

Comment 33

[Michelle Funches - QA]

Version 	48.0a1
Build ID 	20160321030217
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:48.0) Gecko/20100101 Firefox/48.0
Video DownloadHelper	5.5.0a17	true	{b9db16a4-6edc-47ec-a1f4-b86292ed211d}
When E10s is enabled the Video Helper fails to function. Browser Console  displays "This method is not supported with E10s"
Confirm: Is this AddOn supported with Mac (10.11)? Installs but does not displays the rotating icon.

Comment 34

[Michel Gutierrez]

This relates to bug #791098 and calls from the Video DownloadHelper add-on to tab.getThumbnail().

This call is not essential to the addon and it handles the function returning some null value or throwing an exception. The only problem is the console error message left by the tab.getThumbnail() implementation when in e10s: "This method is not supported with E10S"

If/when there is an async replacement for tab.getThumbnail() it should be replaced. Until then, the function should not be called if we know it will fail (we are in e10s).

Is there a recommended way for an add-on to check for e10s being active ?

Comment 35

[Giorgio Maone [:ma1]]

(In reply to Michel Gutierrez from comment #34)
> This relates to bug #791098 and calls from the Video DownloadHelper add-on

> Is there a recommended way for an add-on to check for e10s being active ?

let e10sEnabled = Services.appinfo.browserTabsRemoteAutostart;

Comment 36

[Michel Gutierrez]

Many thanks Giorgio ! Your trick worked fine for me.

Previously, i was checking for tabView.getAttribute('remote') to call tab.getThumbnail but this was apparently not a solid enough method.

The fix will be in 5.5.0.

Comment 37

[:shell escalante]

Hi Michael,

Just checking - when 5.5.0 releases ... will Video downloadhelper work with e10s WITHOUT shims :)

Comment 38

[Michel Gutierrez]

I'm not quite sure what kind of shims i should be looking for in the code ?

Comment 39

[Giorgio Maone [:ma1]]

(In reply to Michel Gutierrez from comment #38)
> I'm not quite sure what kind of shims i should be looking for in the code ?

Shims are not in your code, are in the browser to mimic non-e10s behavior when e10s is enabled.
Specifically, the most common case when your code depends on shims is when it touches DOM objects or windows from the content process in the parent process, or when you register an observer / content policy in the "wrong" process.

To opt-out from shims and check whether your code still works, set the "multiprocess" permission to true in your package.json file: https://developer.mozilla.org/en-US/Add-ons/SDK/Tools/package_json

Comment 40

[Michel Gutierrez]

Thanks Giorgio, that information was very helpful.

I set "multiprocess" to true in package.json/permissions and ran basic tests on the add-on. Everything worked as expected (i have rewritten the entire add-on one year ago to be architecturally e10s-comptatible) on Fx 48 with e10s enabled.

BUT

I ran into a e10s-related problem which prevents VDH (5.5.0xx) to capture chunked video streams on some sites.

The add-on monitors requests/responses to find videos. When a response looks like a HLS or DASH video manifest (based on requested URL and/or response Content-Type), a listener is added to the channel (from the nsITraceableChannel interface) to get the actual manifest content.

If the server sends plain text data, it works with e10s enabled or not.
If the server sends gzip-encoded data:
- with e10s disabled: the listener gets called with decoded data
- with e10s enabled: the listener gets encoded data

I couldn't find a bug opened on this topic. I will fill in a bug about that issue.


Video DownloadHelper 5.5.0 will be released with permissions/multiprocess set to true and is assumed to handle e10s without shim.

Comment 41

[Michel Gutierrez]

For information, 5.5.0 has been submitted to amo. Apart for the blocking bug #1261585 (which only causes issues on a few sites), it is working with e10s enabled and declared as such.

Comment 42

[Dave Miller [:justdave]]

shouldn't this bug be resolved? I have 5.6.1 installed.

Comment 43

[Michelle Funches - QA]

QA Confirmation
Version 	49.0a1
Build ID 	20160606030219
User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0
Video DownloadHelper	5.6.1
E10s; including video from Daily Motion and youtube
also checked on Mac 10.11

Comment 44

[Michel Gutierrez]

Bug #1261585 is still blocking. When e10s is enabled, it prevents VDH to download from sites using HLS or DASH streaming and gzip-encoding the video manifest.

If bug #1261585 is not to be fixed, i can probably write a workaround on my side by decoding the intercepted data manually, but it makes more sense to fix the problem in the API itself.