1275398 - Use shared memory to send images in IPCDataTransfer

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Andrew McCreight [:mccr8]]

Similar to bug 1272018, IPCDataTransfer (which is used by InvokeDragSession, SetClipboard and GetClipboard) can include image data encoded as an nsCString, in IPCDataTransferData. It would be good to use shared memory to transfer this image data, too.

Here's an example of a bug 1271102 crash in SetClipboard:
https://crash-stats.mozilla.com/report/index/dee8a5af-dcdb-4994-b505-5f5c42160524

Comment 1

[Cervantes Yu [:cyu] [:cervantes]]

This can be built on top of the patch for bug 1272018. I'll take it.

Comment 2

[Andrew McCreight [:mccr8]]

Thanks for working on these two bugs!

Comment 3

[Cervantes Yu [:cyu] [:cervantes]]

Attachment: WIP

Comment 4

[Cervantes Yu [:cyu] [:cervantes]]

Attachment: MozReview Request: Bug 1275398 - Use shmem for sending image data in IPCDataTransfer. r?mccr8

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

Comment 5

[Andrew McCreight [:mccr8]]

Comment on attachment 8758676 [details]
MozReview Request: Bug 1275398 - Use shmem for sending image data in IPCDataTransfer. r?mccr8

This should also get reviewed by somebody who understands shmem IPC.

Comment 6

[Nicolas Silva [:nical]]

Comment on attachment 8758676 [details]
MozReview Request: Bug 1275398 - Use shmem for sending image data in IPCDataTransfer. r?mccr8

https://reviewboard.mozilla.org/r/56900/#review53930

Comment 7

[Cervantes Yu [:cyu] [:cervantes]]

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

Comment 8

[Cervantes Yu [:cyu] [:cervantes]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=04042642eef6

Comment 9

[Cervantes Yu [:cyu] [:cervantes]]

GPU test failures don't seem relevant. But there is an assertion failure in https://treeherder.mozilla.org/logviewer.html#?job_id=22167395&repo=try#L4874:

 03:06:13     INFO -  810 INFO TEST-START | dom/events/test/test_bug1264380.html
 03:06:13     INFO -  ++DOMWINDOW == 45 (0x11d971400) [pid = 2098] [serial = 45] [outer = 0x11aae2c00]
 03:06:13     INFO -  Assertion failure: mSegment (null segment), at /builds/slave/try-m64-d-00000000000000000000/build/src/ipc/glue/Shmem.cpp:288

that needs investigation.

Comment 10

[Cervantes Yu [:cyu] [:cervantes]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4526172c1bda6e9985078bf1462a4f5d1d53dbe6
Bug 1275398 - Use shmem for sending image data in IPCDataTransfer. r=nical

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4526172c1bda

Comment 12

[Chris Peterson [:cpeterson]]

Cervantes, should we uplift this e10s 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.

Comment 13

[Cervantes Yu [:cyu] [:cervantes]]

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 14

[Cervantes Yu [:cyu] [:cervantes]]

Comment on attachment 8758676 [details]
MozReview Request: Bug 1275398 - Use shmem for sending image data in IPCDataTransfer. r?mccr8

Approval Request Comment
[Feature/regressing bug #]: No bug. e10s internals for transferring copy/paste data.
[User impact if declined]: Tab crashes if the user copies/pastes very large data. This is not usual, 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 paste the data. Restarting the browser may help if the allocation of large shared memory succeeds after restart.
[String/UUID change made/needed]: None.

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

If this is easily reproducible and testable we may want to uplift to 48 since this sounds like a bad crash. When you say even restarting the browser doesn't help, what do you mean? That trying to copy and paste again will just crash again every time, or is there some other result? 
Can you suggest ways that QA can test the fix once it lands on aurora?

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8758676 [details]
MozReview Request: Bug 1275398 - Use shmem for sending image data in IPCDataTransfer. r?mccr8

Potential e10s crash fix, please uplift to aurora.

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d36dbe26035d

Comment 18

[Cervantes Yu [:cyu] [:cervantes]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #15)
> If this is easily reproducible and testable we may want to uplift to 48
> since this sounds like a bad crash. When you say even restarting the browser
> doesn't help, what do you mean? That trying to copy and paste again will
I said this because of the limit we put in e10s internals in bug 1271102. It placed a fixed limit of max message size of IPC message. On branches with that patch, the user will *always* crash in trying to paste data more that result in an IPC message > 256 MB (like a very large photo). Because the limit is fixed, the user has no way to work it around: none of restarting the browser, freeing more RAM from the system, etc. will work. It will just crash each time the user tries to paste this data. Bug 1271102 lands on 49 and 50 so this bug needs to land on 49 and 50 so copy/paste doesn't crash on very large data. 48 doesn't have bug 1271102 so it won't crash on copy/paste very large data. It's just a little slower than branches that has this patch.

> just crash again every time, or is there some other result? 
> Can you suggest ways that QA can test the fix once it lands on aurora?
Tests should be done by copy/paste very large data, e.g. a photo > 256 MB or text of similar size.

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

OK, thanks for the explanation!