Closed Bug 1403127 Opened 4 years ago Closed 4 years ago
Can't save an Mp4 video from context menu in full screen mode
59 bytes, text/x-review-board-request
[Affected versions]: Firefox 57.0b3 (Build Id:20170925150345) Firefox 58.0a1 (Build Id:20170925220207) Firefox 56.0 (Build Id:20170925181605) [Affected platforms]: Windows 10 x64 Mac OS X 10.12 Ubuntu 16.04 x86 [Steps to reproduce]: 1.Launch Firefox. 2.Got to https://goo.gl/K9rrxQ webpage. 3.Enter full screen mode and right click for access the context menu, click on "Save Video As" menu item [Expected result]: File Explorer window appear to enter name of file to save to... [Actual result]: Nothing happens. The File Explorer window not appear and the video can't be saved from context menu. [Regression range]: This seems like an recent regression and we'll return with more info as soon as possible. [Additional notes]: On Firefox 56.0 - build5 this issue can't reproduced and on latest nightly this issue is reproduced.
Ray, Can you help check if that file explorer windows is triggered? Another guess is that window does appear but it may be covered by other layers, so we cannot see it.
I couldn't reproduce the issue on 58.0a1 (2017-09-26) (64-bit) Mac OS, always get file explorer opened. I think we'll need more information like regression window to tell the possible cause. My guess, if this case affects all the platforms, it might be somewhere error in the glue to native file explorer.
The following error is shown in Browser Console hen click on "Save Video As": Sending message that cannot be cloned. Are you trying to send an XPCOM object? ContextMenu.jsm:624:6 uncaught exception: Load of https://goo.gl/K9rrxQ denied. (unknown)
ah, it's weird I can spot the problem now. Let me look more about ctxmenu, still a regression windows is better to have to know exact regression.
error might be around here: http://searchfox.org/mozilla-central/rev/3dbb47302e114219c53e99ebaf50c5cb727358ab/browser/base/content/nsContextMenu.js#1228-1233 Hi Xidorn, Do we prevent content from opening file explore in fullscreen mode? any different privilege? Thanks
Regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=604fd3302562dc2059a8de3231818a1b618b8ffe&tochange=bf7793529f82ab9cb885a62446ed0f3e18c51634 Suspect : Bug 1360406 @jiangperry, Your bunch of patch seems to cause the regression, Can you look at this?
[Tracking Requested - why for this release]: Regression context menu in Fullscreen mode
More links to reproduced this issue: WebM video sample https://www.quirksmode.org/html5/videos/big_buck_bunny.webm, OGV video sample https://goo.gl/WZJ5NY
I'm not aware of any special privilege requirement for doing anything in fullscreen. I have no idea why this happens only on fullscreen... Maybe because the chrome are all hidden?
So this happens on fullscreen-only because the data.context.target.ownerDocument.fullscreenElement is null when not in fullscreen, but an element in fullscreen. This seems to be the only meaningful difference between the two data being sent.
OK, I think it's easy to fix.
Assignee: nobody → xidorn+moz
Hmmm, we probably should also add a test for this.
But I have no idea how to write test for this kind of functionality... Sounds non-trivial :/
Comment on attachment 8912534 [details] Bug 1403127 - Don't try to pass fullscreen element through IPC. https://reviewboard.mozilla.org/r/183836/#review189254 If you want to write a test, you could look at the following tests: /browser/base/content/test/general/browser_domFullscreen_fullscreenMode.js (this test puts the browser in and out of fullscreen) /browser/base/content/test/general/browser_contextMenu.js (this tests the context menu) For testing that the Save As dialog appears, you could overwrite nsContextMenu.saveHelper and just have that set some test variable when it is called. I'm not sure if that would test enough of the code path that you're looking for. Overwriting nsContextMenu.saveHelper and then restoring it at the end of the test seems the simplest route.
Attachment #8912534 - Flags: review?(jaws) → review+
I... kinda think we should have test, but I'm not very motivated to write it myself :/ If you don't require that, let's just land the patch :P
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/f4b5a83e20e9 Don't try to pass fullscreen element through IPC. r=jaws
Hi Xidorn, should we uplift this fix to Beta57? Sad to see comment 16 :(
Comment on attachment 8912534 [details] Bug 1403127 - Don't try to pass fullscreen element through IPC. Approval Request Comment [Feature/Bug causing the regression]: bug 1360406 [User impact if declined]: may not be able to save video when in fullscreen [Is this code covered by automated tests?]: no [Has the fix been verified in Nightly?]: just landed [Needs manual test from QE? If yes, steps to reproduce]: 1. open a video file (e.g. those in comment 8), 2. click the full screen button at the right bottom corner of the video to enter fullscreen, 3. right click the video to show the context menu 4. click "Save Video As..." [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: no [Why is the change risky/not risky?]: simple and straightforward change [String changes made/needed]: n/a
Attachment #8912534 - Flags: approval-mozilla-beta?
Filed bug 1404159 for tracking adding the test. I may or may not come to that at some point...
I can confirm this issue is fixed on Firefox nightly I verified using Firefox 58.0a1 Win 8.1 x64, Mac 10.12 and Ubuntu 14.04.
Comment on attachment 8912534 [details] Bug 1403127 - Don't try to pass fullscreen element through IPC. Fix a recent regression, taking it. A test would be indeed great. Should be in 57b5
Attachment #8912534 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I can confirm this issue is fixed on Firefox 57.0b5 I verified on Win 8.1 x64, Mac 10.11 and Ubuntu 14.04 x64.
You need to log in before you can comment on or make changes to this bug.