Closed
Bug 1403127
Opened 7 years ago
Closed 7 years ago
Can't save an Mp4 video from context menu in full screen mode
Categories
(Core :: Audio/Video: Playback, defect, P2)
Tracking
()
VERIFIED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox55 | --- | unaffected |
firefox56 | --- | unaffected |
firefox57 | + | verified |
firefox58 | --- | verified |
People
(Reporter: zstimi, Assigned: xidorn)
References
(Blocks 2 open bugs)
Details
(Keywords: multiprocess, regression)
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
jaws
:
review+
Sylvestre
:
approval-mozilla-beta+
|
Details |
[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.
Updated•7 years ago
|
Keywords: regression,
regressionwindow-wanted
Comment 1•7 years ago
|
||
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.
Flags: needinfo?(ralin)
Comment 2•7 years ago
|
||
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.
Flags: needinfo?(ralin)
Comment 3•7 years ago
|
||
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)
Updated•7 years ago
|
Keywords: multiprocess
Comment 4•7 years ago
|
||
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.
Comment 5•7 years ago
|
||
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
Flags: needinfo?(xidorn+moz)
Comment 6•7 years ago
|
||
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?
Updated•7 years ago
|
status-firefox55:
--- → unaffected
status-firefox-esr52:
--- → unaffected
Comment 7•7 years ago
|
||
[Tracking Requested - why for this release]: Regression context menu in Fullscreen mode
tracking-firefox57:
--- → ?
Updated•7 years ago
|
Priority: -- → P2
Reporter | ||
Comment 8•7 years ago
|
||
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
Assignee | ||
Comment 9•7 years ago
|
||
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?
Flags: needinfo?(xidorn+moz)
Assignee | ||
Comment 10•7 years ago
|
||
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.
Assignee | ||
Updated•7 years ago
|
Blocks: dom-fullscreen-ui
Comment hidden (mozreview-request) |
Assignee | ||
Comment 14•7 years ago
|
||
But I have no idea how to write test for this kind of functionality... Sounds non-trivial :/
Comment 15•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 16•7 years ago
|
||
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
Comment 17•7 years ago
|
||
Pushed by xquan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f4b5a83e20e9 Don't try to pass fullscreen element through IPC. r=jaws
Comment 18•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f4b5a83e20e9
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Hi Xidorn, should we uplift this fix to Beta57? Sad to see comment 16 :(
Flags: needinfo?(xidorn+moz)
Assignee | ||
Comment 20•7 years ago
|
||
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
Flags: needinfo?(xidorn+moz)
Flags: needinfo?(jiangperry)
Attachment #8912534 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 21•7 years ago
|
||
Filed bug 1404159 for tracking adding the test. I may or may not come to that at some point...
Reporter | ||
Comment 22•7 years ago
|
||
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.
Updated•7 years ago
|
Status: RESOLVED → VERIFIED
Comment 23•7 years ago
|
||
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+
Comment 24•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/c786424818ea
Reporter | ||
Comment 25•7 years ago
|
||
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.
Description
•