Closed Bug 930788 Opened 11 years ago Closed 8 years ago

[e10s] Video DownloadHelper doesn't work with e10s

Categories

(Firefox :: Extension Compatibility, defect)

defect
Not set
normal

Tracking

()

RESOLVED WORKSFORME
Tracking Status
e10s + ---

People

(Reporter: cpeterson, Assigned: ma1)

References

(Blocks 1 open bug, )

Details

(Keywords: addon-compat, dogfood, Whiteboard: [e10s-top-addon])

Attachments

(1 file)

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
Summary: [e10s] Video DownloadHelper addon support for e10s → [e10s] Video DownloadHelper addon can't download YouTube Flash videos
Attached patch some fixes (deleted) — — Splinter Review
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.
tracking-e10s: --- → +
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → WORKSFORME
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
Depends on: 1040972
Resolution: WORKSFORME → ---
Assignee: evilpies → nobody
e10s isn't enabled by default yet even in Nightly, so clearing the status
No longer blocks: old-e10s-m2
cpeterson, is this still re-producing?
Flags: needinfo?(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
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]
Keywords: dogfood
The developer has been contacted through AMO.
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 ?
(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.
Telemetry is reporting that VDH is regularly throwing a js exception in util-service.jsm line 322.
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.
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.
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
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.
My latest development code seems to work (basic tests only) on the last nightly and aurora. Can i assume it is e10s compliant ?
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.
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 :)
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 ?
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)
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)
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.
Taking ownership of this bug and working more closely with Michel to help fixing the remaining issues.
Assignee: nobody → g.maone
That's excellent news ! Thanks Giorgio.
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
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
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.
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 → ---
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 ?
(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;
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.
Hi Michael, Just checking - when 5.5.0 releases ... will Video downloadhelper work with e10s WITHOUT shims :)
Flags: needinfo?(michel.gutierrez)
I'm not quite sure what kind of shims i should be looking for in the code ?
Flags: needinfo?(michel.gutierrez)
(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
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.
Depends on: 1261585
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
Closed: 10 years ago8 years ago
Resolution: --- → WORKSFORME
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.

Attachment

General

Created:
Updated:
Size: