Can't save an Mp4 video from context menu in full screen mode

VERIFIED FIXED in Firefox 57

Status

()

P2
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: zstimi, Assigned: xidorn)

Tracking

(Blocks: 2 bugs, {multiprocess, regression})

57 Branch
mozilla58
multiprocess, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 unaffected, firefox57+ verified, firefox58 verified)

Details

Attachments

(1 attachment)

[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.
Keywords: regression, regressionwindow-wanted
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

2 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

2 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

2 years ago
Keywords: multiprocess

Comment 4

2 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

2 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

2 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?
Blocks: 1360406
Flags: needinfo?(jiangperry)
Keywords: regressionwindow-wanted
status-firefox55: --- → unaffected
status-firefox-esr52: --- → unaffected

Comment 7

2 years ago
[Tracking Requested - why for this release]: Regression context menu in Fullscreen mode
tracking-firefox57: --- → ?
Priority: -- → P2
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

2 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

2 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)

Comment 11

2 years ago
OK, I think it's easy to fix.
Assignee: nobody → xidorn+moz
(Assignee)

Updated

2 years ago
Blocks: 1209831
Comment hidden (mozreview-request)
(Assignee)

Comment 13

2 years ago
Hmmm, we probably should also add a test for this.
(Assignee)

Comment 14

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

Comment 16

2 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

2 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f4b5a83e20e9
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Hi Xidorn, should we uplift this fix to Beta57? Sad to see comment 16 :(
Flags: needinfo?(xidorn+moz)
tracking-firefox57: ? → +
(Assignee)

Comment 20

2 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)

Updated

2 years ago
Blocks: 1404159
(Assignee)

Comment 21

2 years ago
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.
status-firefox58: fixed → verified
Status: RESOLVED → VERIFIED
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

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/c786424818ea
status-firefox57: affected → fixed
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.
status-firefox57: fixed → verified
You need to log in before you can comment on or make changes to this bug.