[e10s] Video DownloadHelper doesn't work with e10s

RESOLVED WORKSFORME

Status

()

RESOLVED WORKSFORME
5 years ago
11 months ago

People

(Reporter: cpeterson, Assigned: mao)

Tracking

(Blocks: 1 bug, {addon-compat, dogfood})

unspecified
addon-compat, dogfood
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+)

Details

(Whiteboard: [e10s-top-addon], URL)

Attachments

(1 attachment)

(Reporter)

Comment 1

5 years ago
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.
Depends on: 946583
(Reporter)

Updated

5 years ago
Summary: [e10s] Video DownloadHelper addon support for e10s → [e10s] Video DownloadHelper addon can't download YouTube Flash videos
Created attachment 8358091 [details] [diff] [review]
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.
(Reporter)

Updated

4 years ago
tracking-e10s: --- → +
Blocks: 997462
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → WORKSFORME
(Reporter)

Comment 3

4 years ago
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.
Status: RESOLVED → REOPENED
status-firefox31: --- → affected
status-firefox32: --- → ?
status-firefox33: --- → ?
Depends on: 1040972
Resolution: WORKSFORME → ---
(Reporter)

Updated

4 years ago
See Also: → bug 946594
Assignee: evilpies → nobody
e10s isn't enabled by default yet even in Nightly, so clearing the status
No longer blocks: 997462
status-firefox31: affected → ---
status-firefox32: ? → ---
status-firefox33: ? → ---
cpeterson, is this still re-producing?
Flags: needinfo?(cpeterson)
(Reporter)

Comment 6

4 years ago
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
Flags: needinfo?(cpeterson)
Summary: [e10s] Video DownloadHelper addon can't download YouTube Flash videos → [e10s] Video DownloadHelper addon can't download DailyMotion Flash videos
Whiteboard: [e10s-top-addon]
(Reporter)

Updated

4 years ago
Keywords: dogfood
The developer has been contacted through AMO.

Comment 8

4 years ago
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 ?
(Reporter)

Comment 9

4 years ago
(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.
Blocks: 1111730
Telemetry is reporting that VDH is regularly throwing a js exception in util-service.jsm line 322.

Comment 11

4 years ago
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

4 years ago
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

4 years ago
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" ?
Summary: [e10s] Video DownloadHelper addon can't download DailyMotion Flash videos → [e10s] Video DownloadHelper doesn't work with e10s

Comment 14

4 years ago
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

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

Comment 16

4 years ago
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

4 years ago
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

4 years ago
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 ?
(Reporter)

Comment 19

4 years ago
Jim, do you have any suggestions for how the Video DownloadHelper add-on can monitor HTTP channels in e10s?
Flags: needinfo?(jmathies)
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?
Flags: needinfo?(jmathies) → needinfo?(dtownsend)

Comment 21

4 years ago
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.
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.
Flags: needinfo?(dtownsend)

Comment 23

4 years ago
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.
(Assignee)

Comment 24

3 years ago
Taking ownership of this bug and working more closely with Michel to help fixing the remaining issues.
Assignee: nobody → g.maone

Comment 25

3 years ago
That's excellent news ! Thanks Giorgio.
(Assignee)

Comment 26

3 years ago
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.
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago3 years ago
Resolution: --- → FIXED

Comment 27

3 years ago
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

3 years ago
Version 5.2.0 with e10s fix submitted to amo.
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.
(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?
Flags: needinfo?(dtownsend)
(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.
Flags: needinfo?(dtownsend)
Dave fixed the CPOW support for worker, so this should be a non issue again. (bug 1146926)
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 34

2 years ago
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 ?
(Assignee)

Comment 35

2 years ago
(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

2 years ago
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

2 years ago
Hi Michael,

Just checking - when 5.5.0 releases ... will Video downloadhelper work with e10s WITHOUT shims :)
Flags: needinfo?(michel.gutierrez)

Comment 38

2 years ago
I'm not quite sure what kind of shims i should be looking for in the code ?
Flags: needinfo?(michel.gutierrez)
(Assignee)

Comment 39

2 years ago
(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

2 years ago
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.

Updated

2 years ago
Depends on: 1261585

Comment 41

2 years ago
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.
shouldn't this bug be resolved? I have 5.6.1 installed.
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
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago2 years ago
Resolution: --- → WORKSFORME

Comment 44

2 years ago
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.
You need to log in before you can comment on or make changes to this bug.