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

Status: VERIFIED FIXED

(Core :: Audio/Video: Playback, defect, P2)

Description

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

[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.

Comment 1

[Blake Wu [:bwu][:blakewu]]

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.

Comment 2

[Ray Lin[:ralin]]

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.

Comment 3

[Alice0775 White]

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)

Comment 4

[Ray Lin[:ralin]]

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

[Ray Lin[:ralin]]

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

Comment 6

[Alice0775 White]

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?

Comment 7

[Alice0775 White]

[Tracking Requested - why for this release]: Regression context menu in Fullscreen mode

Comment 8

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

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

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

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?

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

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.

Comment 11

[Xidorn Quan [:xidorn] UTC+11]

OK, I think it's easy to fix.

Comment 12

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1403127 - Don't try to pass fullscreen element through IPC.

Review commit: https://reviewboard.mozilla.org/r/183836/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/183836/

Comment 13

[Xidorn Quan [:xidorn] UTC+11]

Hmmm, we probably should also add a test for this.

Comment 14

[Xidorn Quan [:xidorn] UTC+11]

But I have no idea how to write test for this kind of functionality... Sounds non-trivial :/

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 16

[Xidorn Quan [:xidorn] UTC+11]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f4b5a83e20e9
Don't try to pass fullscreen element through IPC. r=jaws

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f4b5a83e20e9

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Xidorn, should we uplift this fix to Beta57? Sad to see comment 16 :(

Comment 20

[Xidorn Quan [:xidorn] UTC+11]

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

Comment 21

[Xidorn Quan [:xidorn] UTC+11]

Filed bug 1404159 for tracking adding the test. I may or may not come to that at some point...

Comment 22

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

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 23

[Sylvestre Ledru [:Sylvestre]]

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

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c786424818ea

Comment 25

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

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.