SVG images break the context menu because `imageInfo` contains SVGAnimated properties which are not serializable
Categories
(Firefox :: Menus, defect, P1)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox75 | --- | verified |
People
(Reporter: wtds.trabalho, Assigned: Gijs)
References
()
Details
(Keywords: regression)
Attachments
(6 files)
cant-report-site-issue.png
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Firefox/68.0
Steps to reproduce:
-
Try to right click and save images
-
See many problems on use browser functions like the function "Report Site Problem"
Actual results:
-
Attached.
-
Errors(Right click and save/view image):
JavaScript error: resource://gre/modules/BrowserUtils.jsm, line 91: Error: Load of https://s2.glbimg.com/ww5zUd6nwJFAmBMQQWcmyFnSsG0=/620x480/e.glbimg.com/og/ed/f/original/2020/01/06/.img_6710.jpg denied.
JavaScript error: resource://gre/modules/BrowserUtils.jsm, line 91: Error: Load of https://s2.glbimg.com/ww5zUd6nwJFAmBMQQWcmyFnSsG0=/620x480/e.glbimg.com/og/ed/f/original/2020/01/06/.img_6710.jpg denied.
JavaScript error: resource://gre/modules/BrowserUtils.jsm, line 91: Error: Load of https://s2.glbimg.com/ww5zUd6nwJFAmBMQQWcmyFnSsG0=/620x480/e.glbimg.com/og/ed/f/original/2020/01/06/.img_6710.jpg denied.
JavaScript error: resource://gre/modules/BrowserUtils.jsm, line 91: Error: Load of https://s2.glbimg.com/ww5zUd6nwJFAmBMQQWcmyFnSsG0=/620x480/e.glbimg.com/og/ed/f/original/2020/01/06/.img_6710.jpg denied.
...
- And(Screenshot):
JavaScript error: data:,function%20getInfoFrameScript(messageName)%20%7B%0A%20%20/*%20eslint-env%20mozilla/frame-script%20*/%0A%0A%20%20const%20%7B%20Services%20%7D%20=%20ChromeUtils.import(%0A%20%20%20%20%22resource://gre/modules/Services.jsm%22%0A%20%20);%0A%20%20const%20PREVIEW_MAX_ITEMS%20=%2010;%0A%20%20const%20LOG_LEVEL_MAP%20=%20%7B%0A%20%20%20%200:%20%22debug%22,%0A%20%20%20%201:%20%22info%22,%0A%20%20%20%202:%20%22warn%22,%0A%20%20%20%203:%20%22error%22,%0A%20%20%7D;%0A%0A%20%20function%20getInnerWindowId(window)%20%7B%0A%20%20%20%20return%20window.windowUtils.currentInnerWindowID;%0A%20%20%7D%0A%0A%20%20function%20getInnerWindowIDsForAllFrames(window)%20%7B%0A%20%20%20%20const%20innerWindowID%20=%20getInnerWindowId(window);%0A%20%20%20%20let%20ids%20=%20%5BinnerWindowID%5D;%0A%0A%20%20%20%20if%20(window.frames)%20%7B%0A%20%20%20%20%20%20for%20(let%20i%20=%200;%20i%20%3C%20window.frames.length;%20i++)%20%7B%0A%20%20%20%20%20%20%20%20ids%20=%20ids.concat(getInnerWindowIDsForAllFrames(window.frames%5Bi%5D));%0A%20%20%20%20%20%20%7D%0A%20%20%20%20%7D%0A%0A%20%20%20%20return%20ids;%0A%20%20%7D%0A%0A%20%20function%20getLoggedMessages(window,%20includePrivate%20=%20false)%20%7B%0A%20%20%20%20const%20ids%20=%20getInnerWindowIDsForAllFrames(window);%0A%20%20%20%20return%20getConsoleMessages(ids)%0A%20%20%20%20%20%20.concat(getScriptErrors(ids,%20includePrivate))%0A%20%20%20%20%20%20.sort((a,%20b)%20=%3E%20a.timeStamp%20-%20b.timeStamp)%0A%20%20%20%20%20%20.map(m%20=%3E%20m.message);%0A%20%20%7D%0A%0A%20%20function%20getPreview(value)%20%7B%0A%20%20%20%20switch%20(typeof%20value)%20%7B%0A%20%20%20%20%20%20case%20%22function%22:%0A%20%20%20%20%20%20%20%20return%20%22function%20()%22;%0A%0A%20%20%20%20%20%20case%20%22object%22:%0A%20%20%20%20%20%20%20%20if%20(value%20===%20null)%20%7B%0A%20%20%20%20%20%20%20%20%20%20return%20null;%0A%20%20%20%20%20%20%20%20%7D%0A%0A%20%20%20%20%20%20%20%20if%20(Array.isArray(value))%20%7B%0A%20%20%20%20%20%20%20%20%20%20return%20%60($%7Bvalue.length%7D)%5B...%5D%60;%0A%20%20%20%20%20%20%20%20%7D%0A%0A%20%20%20%20%20%20%20%20return%20%22%7B...%7D%22;%0A%0A%20%20%20%20%20%20case%20%22undefined%22:%0A%20%20%20%20%20%20%20%20return%20%22undefined%22;%0A%0A%20%20%20%20%20%20default:%0A%20%20%20%20%20%20%20%20return%20value;%0A%20%20%20%20%7D%0A%20%20%7D%0A%0A%20%20function%20getArrayPreview(arr)%20%7B%0A%20%20%20%20const%20preview%20=%20%5B%5D;%0A%20%20%20%20for%20(const%20value%20of%20arr)%20%7B%0A%20%20%20%20%20%20preview.push(getPreview(value));%0A%0A%20%20%20%20%20%20if%20(preview.length%20===%20PREVIEW_MAX_ITEMS)%20%7B%0A%20%20%20%20%20%20%20%20break;%0A%20%20%20%20%20%20%7D%0A%20%20%20%20%7D%0A%0A%20%20%20%20return%20preview;%0A%20%20%7D%0A%0A%20%20function%20getObjectPreview(obj)%20%7B%0A%20%20%20%20const%20preview%20=%20%7B%7D;%0A%20%20%20%20for%20(const%20key%20in%20obj)%20%7B%0A%20%20%20%20%20%20if%20(obj.hasOwnProperty(key))%20%7B%0A%20%20%20%20%20%20%20%20preview%5Bkey%5D%20=%20getPreview(obj%5Bkey%5D);%0A%20%20%20%20%20%20%7D%0A%0A%20%20%20%20%20%20if%20(Object.keys(preview).length%20===%20PREVIEW_MAX_ITEMS)%20%7B%0A%20%20%20%20%20%20%20%20break;%0A%20%20%20%20%20%20%7D%0A%20%20%20%20%7D%0A%0A%20%20%20%20return%20preview;%0A%20%20%7D%0A%0A%20%20function%20getArgs(value)%20%7B%0A%20%20%20%20if%20(typeof%20value%20===%20%22object%22%20&&%20value%20!==%20null)%20%7B%0A%20%20%20%20%20%20if%20(Array.isArray(value))%20%7B%0A%20%20%20%20%20%20%20%20return%20getArrayPreview(value);%0A%20%20%20%20%20%20%7D%0A%0A%20%20%20%20%20%20return%20getObjectPreview(value);%0A%20%20%20%20%7D%0A%0A%20%20%20%20return%20getPreview(value);%0A%20%20%7D%0A%0A%20%20function%20getConsoleMessages(windowIds)%20%7B%0A%20%20%20%20const%20ConsoleAPIStorage%20=%20Cc%5B%0A%20%20%20%20%20%20%22@mozilla.org/consoleAPI-storage;1%22%0A%20%20%20%20%5D.getService(Ci.nsIConsoleAPIStorage);%0A%20%20%20%20let%20messages%20=%20%5B%5D;%0A%20%20%20%20for%20(const%20id%20of%20windowIds)%20%7B%0A%20%20%20%20%20%20messages%20=%20messages.concat(ConsoleAPIStorage.getEvents(id)%20%7C%7C%20%5B%5D);%0A%20%20%20%20%7D%0A%20%20%20%20return%20messages.map(evt%20=%3E%20%7B%0A%20%20%20%20%20%20const%20%7B%20columnNumber,%20filename,%20level,%20lineNumber,%20timeStamp%20%7D%20=%20evt;%0A%20%20%20%20%20%20const%20args%20=%20evt.arguments.map(getArgs);%0A%0A%20%20%20%20%20%20const%20message%20=%20%7B%0A%20%20%20%20%20%20%20%20level,%0A%20%20%20%20%20%20%20%20log:%20args,%0A%20%20%20%20%20%20%20%20uri:%20filename,%0A%20%20%20%20%20%20%20%20pos:%20%60$%7BlineNumber%7D:$%7BcolumnNumber%7D%60,%0A%20%20%20%20%20%20%7D;%0A%0A%20%20%20%20%20%20return%20%7B%20timeStamp,%20message%20%7D;%0A%20%20%20%20%7D);%0A%20%20%7D%0A%0A%20%20function%20getScriptErrors(windowIds,%20includePrivate%20=%20false)%20%7B%0A%20%20%20%20const%20messages%20=%20Services.console.getMessageArray()%20%7C%7C%20%5B%5D;%0A%20%20%20%20return%20messages%0A%20%20%20%20%20%20.filter(message%20=%3E%20%7B%0A%20%20%20%20%20%20%20%20if%20(message%20instanceof%20Ci.nsIScriptError)%20%7B%0A%20%20%20%20%20%20%20%20%20%20if%20(!includePrivate%20&&%20message.isFromPrivateWindow)%20%7B%0A%20%20%20%20%20%20%20%20%20%20%20%20return%20false;%0A%20%20%20%20%20%20%20%20%20%20%7D%0A%0A%20%20%20%20%20%20%20%20%20%20if%20(windowIds%20&&%20!windowIds.includes(message.innerWindowID))%20%7B%0A%20%20%20%20%20%20%20%20%20%20%20%20return%20false;%0A%20%20%20%20%20%20%20%20%20%20%7D%0A%0A%20%20%20%20%20%20%20%20%20%20return%20true;%0A%20%20%20%20%20%20%20%20%7D%0A%0A%20%20%20%20%20%20%20%20//%20If%20this%20is%20not%20an%20nsIScriptError%20and%20we%20need%20to%20do%20window-based%0A%20%20%20%20%20%20%20%20//%20filtering%20we%20skip%20this%20message.%0A%20%20%20%20%20%20%20%20return%20false;%0A%20%20%20%20%20%20%7D)%0A%20%20%20%20%20%20.map(error%20=%3E%20%7B%0A%20%20%20%20%20%20%20%20const%20%7B%0A%20%20%20%20%20%20%20%20%20%20timeStamp,%0A%20%20%20%20%20%20%20%20%20%20errorMessage,%0A%20%20%20%20%20%20%20%20%20%20sourceName,%0A%20%20%20%20%20%20%20%20%20%20lineNumber,%0A%20%20%20%20%20%20%20%20%20%20columnNumber,%0A%20%20%20%20%20%20%20%20%20%20logLevel,%0A%20%20%20%20%20%20%20%20%7D%20=%20error;%0A%20%20%20%20%20%20%20%20const%20message%20=%20%7B%0A%20%20%20%20%20%20%20%20%20%20level:%20LOG_LEVEL_MAP%5BlogLevel%5D,%0A%20%20%20%20%20%20%20%20%20%20log:%20%5BerrorMessage%5D,%0A%20%20%20%20%20%20%20%20%20%20uri:%20sourceName,%0A%20%20%20%20%20%20%20%20%20%20pos:%20%60$%7BlineNumber%7D:$%7BcolumnNumber%7D%60,%0A%20%20%20%20%20%20%20%20%7D;%0A%0A%20%20%20%20%20%20%20%20return%20%7B%20timeStamp,%20message%20%7D;%0A%20%20%20%20%20%20%7D);%0A%20%20%7D%0A%0A%20%20docShell.getHasTrackingContentBlocked().then(hasTrackingContentBlocked%20=%3E%20%7B%0A%20%20%20%20sendAsyncMessage(messageName,%20%7B%0A%20%20%20%20%20%20hasMixedActiveContentBlocked:%20docShell.hasMixedActiveContentBlocked,%0A%20%20%20%20%20%20hasMixedDisplayContentBlocked:%20docShell.hasMixedDisplayContentBlocked,%0A%20%20%20%20%20%20hasTrackingContentBlocked,%0A%20%20%20%20%20%20log:%20getLoggedMessages(content),%0A%20%20%20%20%7D);%0A%20%20%7D);%0A%7D;getInfoFrameScript(%22WebExtension:GetWebcompatInfo%22), line 16: SecurityError: Permission denied to access property "windowUtils" on cross-origin object
Expected results:
No problems...
Thanks!
|
Reporter | |
Comment 1•4 years ago
|
||
|
|
Reporter | |
Comment 2•4 years ago
|
||
|
|
Reporter | |
Comment 3•4 years ago
|
||
|
|
Reporter | |
Comment 4•4 years ago
|
||
|
||
Comment 5•4 years ago
|
||
Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:74.0) Gecko/20100101 Firefox/74.0
20200206214720
Tentatively triaging to Toolkit :: General because of the BrowserUtils.jsm error on Save Image and View Image. Also reproducible in Release, so not a recent regression.
|
Assignee | |
Comment 6•4 years ago
|
||
Security checks fail because the principal doesn't arrive in the parent process, because something isn't serializing/deserializing correctly. When structured cloning fails we use JSON to stringify/destringify the information, but this is lossy, so we end up without the principal.
It turns out that the imageInfo object created at https://searchfox.org/mozilla-central/rev/1db5ef59eba65d32d6a29a494e87b6078453e559/browser/actors/ContextMenuChild.jsm#962-967 can have SVG animated width/height properties if the image is an SVG, which then aren't serializable and break things. :-\
|
|
Assignee | |
Comment 7•4 years ago
|
||
|
||
Comment 8•4 years ago
|
||
Hey jdai,
So the bug here that we're fixing is that there's a edge-case where a non-serializable object tries to get sent through the JSWindowActors. We throw a warning in that case to the console... I wonder if we should explode in a louder way, maybe in at least debug builds in automation? It'd be super helpful to get the type of the thing that we're trying to send over the wire. Do you know how feasible that would be?
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/27fc34dc2e4d fix context menu handling of SVG images so it works in e10s, r=mconley
|
||
Comment 10•4 years ago
|
||
| bugherder | ||
|
||
Comment 11•4 years ago
|
||
(In reply to Mike Conley (:mconley) (:⚙️) from comment #8)
Hey jdai,
So the bug here that we're fixing is that there's a edge-case where a non-serializable object tries to get sent through the JSWindowActors. We throw a warning in that case to the console... I wonder if we should explode in a louder way, maybe in at least debug builds in automation? It'd be super helpful to get the type of the thing that we're trying to send over the wire. Do you know how feasible that would be?
It's throwing error[1] in the code, it should be catchable in automation if we have a testcase. Did I miss something?
|
|
Assignee | |
Comment 12•4 years ago
•
|
||
(In reply to John Dai[:jdai] from comment #11)
(In reply to Mike Conley (:mconley) (:⚙️) from comment #8)
Hey jdai,
So the bug here that we're fixing is that there's a edge-case where a non-serializable object tries to get sent through the JSWindowActors. We throw a warning in that case to the console... I wonder if we should explode in a louder way, maybe in at least debug builds in automation? It'd be super helpful to get the type of the thing that we're trying to send over the wire. Do you know how feasible that would be?
It's throwing error[1] in the code, it should be catchable in automation if we have a testcase. Did I miss something?
The error is suppressed inside GetParamsForMessage at https://searchfox.org/mozilla-central/rev/0439db3a39faddb90197a87cc184c57dcbf0a770/dom/base/nsFrameMessageManager.cpp#395 and the resulting warning misses useful information. Though even the original exception doesn't seem to have e.g. a path into the object it was passed indicating which property was unserializable. (so if I pass {foo: { bar: {docShell}}} as the data param to sendAsyncMessage, I'd like to be told something like data.foo.bar is of type nsIDocShell and is not serializable).
Edit: and, ideally, on infra that should just throw, not paper over it by using JSON serialization instead.
|
|
||
Comment 13•4 years ago
|
||
(In reply to :Gijs (he/him) from comment #12)
(In reply to John Dai[:jdai] from comment #11)
(In reply to Mike Conley (:mconley) (:⚙️) from comment #8)
Hey jdai,
So the bug here that we're fixing is that there's a edge-case where a non-serializable object tries to get sent through the JSWindowActors. We throw a warning in that case to the console... I wonder if we should explode in a louder way, maybe in at least debug builds in automation? It'd be super helpful to get the type of the thing that we're trying to send over the wire. Do you know how feasible that would be?
It's throwing error[1] in the code, it should be catchable in automation if we have a testcase. Did I miss something?
The error is suppressed inside
GetParamsForMessageat https://searchfox.org/mozilla-central/rev/0439db3a39faddb90197a87cc184c57dcbf0a770/dom/base/nsFrameMessageManager.cpp#395 and the resulting warning misses useful information. Though even the original exception doesn't seem to have e.g. a path into the object it was passed indicating which property was unserializable. (so if I pass{foo: { bar: {docShell}}}as thedataparam tosendAsyncMessage, I'd like to be told something likedata.foo.bar is of type nsIDocShell and is not serializable).
File bug 1616279 to improve it.
Edit: and, ideally, on infra that should just throw, not paper over it by using JSON serialization instead.
The JSON serialization[1] was introduced by bug 759427 comment 40. Per offline discussion with Olli, nsIDOMContactFindOptions was removed long ago. It would be nice to refactor it. :-)
|
||
Updated•4 years ago
|
|
||
Comment 14•4 years ago
|
||
Verified the issue with Firefox 75.0b4 on Windows 10 x64 and Ubuntu 18.04 and it is no longer reproducible.
Updating the flag -> Verified.
|
||
Comment 15•4 years ago
|
||
Bugbug thinks this bug is a regression, but please revert this change in case of error.
|
||
Comment 16•4 years ago
|
||
Based on comment 14 removing the qe+ flag and setting status to verified.
Description
•