Currently, drag and drop sends images by serializing the image data to a string, then copying that string into an IPC message (see the various callers of nsContentUtils::GetSurfaceData). This can result in large IPC messages, as seen in bug 1271102. Shared memory would probably be a better way to do this, and I think we already have some kind of setup for sharing image data via shared memory so maybe it wouldn't be too hard. Generalized drag and drop involves some entire data structure, so maybe it would be.

Kan-ru, could somebody on your team look into this? It sounds like it could be straightforward for somebody who understands how shared memory works for image data.

I can take a look.

Nical, I know we have the mechanism to share surfaces across processes. The content process creates a DataSourceSurface for transmission by copying and the receiver in parent process also creates a DataSourceSurface out of the raw data by copying. I think we can eliminate one of the two copies here by using some kind of shared surface. Do you know if there is something we can reuse?

This uses Shmem for passing the image data. TODO: remove the extra copy in nsDragServiceProxy::InvokeDragSessionImpl(). Even further, find a way to share the surface to even reduce the number of copies so that the child process allocates and fills the shared memory, and the parent process uses the shared memory without making another copy and deallocates it after done with it.

Wait, is this bug about using shared memory for the drag feedback image, or for the actual images being dragged? Or both? The patch suggests the drag feedback image only.

(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #3) > Nical, I know we have the mechanism to share surfaces across processes. The > content process creates a DataSourceSurface for transmission by copying and > the receiver in parent process also creates a DataSourceSurface out of the > raw data by copying. I think we can eliminate one of the two copies here by > using some kind of shared surface. Do you know if there is something we can > reuse? The standard texture sharing mechanism is to create a TextureClient/TextureHost pair. It gives you a way to access it as a DrawTarget on the child side so that you can render what you need in there, and as a TextureSource on the parent side, which is what the Compositor knows how to draw, all of that without extra copy (Assuming we use a Compositor to draw the image when drag-n-drop'ing, I'm not sure, if it's not the case we could make TextureHost expose a SourceSurface too). PContent doesn't manage PTexture so if you want to use it here there's a little it of ipdl plumbing to add but nothing too complicated. The other advantage of use TextureClient/Host is that you are not limited to sharing shmems, you can also use, say, DXGI surfaces if it's more efficient for your use case without adding code. That said, the existing code is already based on using data surfaces and share some buffers, so just replacing the nsCString by a Shmem like in Cervantes's patch is already a notable win with minimal code changes.

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

(In reply to Neil Deakin from comment #6) > Wait, is this bug about using shared memory for the drag feedback image, or > for the actual images being dragged? Or both? The patch suggests the drag > feedback image only. I filed bug 1275398 about the string usage in IPCDataTransferData, which should cover the images being dragged (plus clipboard stuff). I didn't want to pile too much stuff into this bug. They should be fairly orthogonal, though similar code.

Comment on attachment 8756297 [details] MozReview Request: Bug 1272018 - Use shared memory to transfer drag image data. r?mccr8 https://reviewboard.mozilla.org/r/55074/#review52164 ::: gfx/2d/DataSurfaceHelpers.h:12 (Diff revision 1) > #define _MOZILLA_GFX_DATASURFACEHELPERS_H > > #include "2D.h" > > #include "mozilla/UniquePtr.h" > +#include "nsDebug.h" Code in Moz2D can only depend on MFBT classes.

Comment on attachment 8756297 [details] MozReview Request: Bug 1272018 - Use shared memory to transfer drag image data. r?mccr8 This seems to work ok, but I think Andrew would be a better reviewer of this.

Comment on attachment 8756297 [details] MozReview Request: Bug 1272018 - Use shared memory to transfer drag image data. r?mccr8 Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55074/diff/1-2/

Comment on attachment 8756297 [details] MozReview Request: Bug 1272018 - Use shared memory to transfer drag image data. r?mccr8 This should get reviewed by somebody who is familiar with shared memory IPC. The patch looks straightforward otherwise.

(In reply to Andrew McCreight [:mccr8] from comment #13) > This should get reviewed by somebody who is familiar with shared memory IPC. > The patch looks straightforward otherwise. I looked at the patch and the shmem stuff looks good. We have a layers::ShmemAllocator interface identical to the IShmemAllocator added here, so we should remove the one in layers as a followup if this lands.

Comment on attachment 8756297 [details] MozReview Request: Bug 1272018 - Use shared memory to transfer drag image data. r?mccr8 https://reviewboard.mozilla.org/r/55074/#review53928

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b3715489009 Perm fails in GPU tests to be fixed.

(In reply to Cervantes Yu [:cyu] [:cervantes] from comment #16) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b3715489009 > Perm fails in GPU tests to be fixed. You probably just need to update to a newer parent rev to make those gpu failures go away.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d3571746fb0bed108c2e6d7ce5a78fe8374755fd Bug 1272018 - Use shared memory to transfer drag image data. r=nical

Cervantes, should we uplift this e10s bug fix from Nightly 50 to Aurora 49 or even Beta 48? We plan to enable e10s by default for some release channel users with Firefox 48.

Since I think we should uplift this to 49 since for limitation of IPC message size. I would leave 48 as is since this isn't a must-have fix.

Comment on attachment 8756297 [details] MozReview Request: Bug 1272018 - Use shared memory to transfer drag image data. r?mccr8 Approval Request Comment [Feature/regressing bug #]: No bug. e10s internals for transferring drag/drop image data. [User impact if declined]: Tab crashes if the user drags very large image. This is very unusual, but when it happens the user has no workaround. Even restarting the browser doesn't help. [Describe test coverage new/current, TreeHerder]: Landed/autotested on m-c. [Risks and why]: Low to medium. We are not sure whether shared memory has higher chance failing in allocation. If it fails to allocate shared memory, the user will fail to drag an image. Restarting the browser may help if the allocation of large shared memory succeeds after restart. [String/UUID change made/needed]: None.

Thanks. wontfix for Beta 48 based on Cervantes' comment 21.

Comment on attachment 8756297 [details] MozReview Request: Bug 1272018 - Use shared memory to transfer drag image data. r?mccr8 Fix for crashes from large image copying, let's try it in aurora. cyu, do you have any suggested ways to test/verify this fix?

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #24) > Comment on attachment 8756297 [details] > MozReview Request: Bug 1272018 - Use shared memory to transfer drag image > data. r?mccr8 > > Fix for crashes from large image copying, let's try it in aurora. > cyu, do you have any suggested ways to test/verify this fix? Dragging a 70 MP image and see if the tab crashes. Please make sure the system has enough free memory so that it doesn't hit OOM.