1153613 - [e10s] Crash when starting image drag on twitter

Status: VERIFIED FIXED

(Firefox :: Untriaged, defect)

Description

[Greg Karz]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:40.0) Gecko/20100101 Firefox/40.0
Build ID: 20150411030207

Steps to reproduce:

Started to drag an image on twitter


Actual results:

https://crash-stats.mozilla.com/report/index/324fb970-49ad-42c3-b36c-5f2152150412

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Can you reproduce the crash? (I can't on linux nor on Windows)


Oh, perhaps ... 
I was explicitly told that one needs to do http://mxr.mozilla.org/mozilla-central/source/widget/nsDragServiceProxy.cpp?rev=55524bdeb708&mark=63-65,69-69#69
to get the data, but http://mxr.mozilla.org/mozilla-central/source/gfx/2d/SourceSurfaceRawData.cpp?rev=f5ad5364a2df&mark=41-41#32
does then something else when cloning the data.
I was pointed to https://mxr.mozilla.org/mozilla-central/source/gfx/2d/SourceSurfaceCG.cpp?rev=c9e48790f6fa#164
But I see, perhaps I should use the original bufLen for nsDependentCString length after all?
(nsCString is used as a data container for IPC layer here)

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Or, hmm, the size of the buffer needs to be the original bufLen, but only some part of the buffer is valid... Hmm, do I need a memcpy here :/

Comment 3

[Greg Karz]

It is harder to reproduce the problem than I described.

Comment 4

[Alice0775 White]

I can reprodiuce on Windows7.
bp-dcc3af23-bdf9-4c0f-8283-98bd82150412

Reproducible : very often

Steps to reproduced
1. Open http://twitter.com/ikaroskun (you do not need login to twitter)
3. Drag a image at top-left
4. if not crash , toggle applications, and repeat stap3

Actual results:
Crash when drag drop

Comment 5

[Alice0775 White]

http://www.w3schools.com/html/html5_draganddrop.asp demo page is also crashed
bp-34a8e69e-ef5e-435e-85e2-048552150412

Comment 6

[Alice0775 White]

Still 100% reproduce on windows7 e10s with str

Steps to reproduced
1. Open http://twitter.com/ikaroskun [Tab1] (you do not need login to twitter)
2. Open New Tab [Tab2] and back to [Tab1]
3. Wait for 40-60 sec
4. Drag a image at top-left

Browser crashes

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Do you mean the Twitter logo or that yellow-head thing?

Comment 8

[Alice0775 White]

(In reply to Olli Pettay [:smaug] from comment #7)
> Do you mean the Twitter logo or that yellow-head thing?

The yellow-head

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v1

I can't reproduce this, but based on the code inspection we need
something like this anyway.

I'm not super happy with always copying the data, but not very worried either, for now at least.
These images aren't anything very big. (but thinking still how to optimize this. If SourceSurface just had nicer API...)

Alice, could you perhaps try the tryserver build once it is ready?

Comment 11

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8591937 [details] [diff] [review]
v1

Review of attachment 8591937 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/nsDragServiceProxy.cpp
@@ +72,5 @@
> +
> +      // nsDependentCString wants null-terminated string.
> +      mozilla::UniquePtr<char[]> dragImageData(new char[maxBufLen + 1]);
> +      memcpy(dragImageData.get(), reinterpret_cast<char*>(map.mData), bufLen);
> +      memset(dragImageData.get() + bufLen, 0, maxBufLen - bufLen + 1);

Why are we using maxBufLen instead of just bufLen for these? It seems unnecessary to copy the last row of padding, and we weren't making this available before.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Because the parent process then uses the data when creating its SourceSurface and relies on
the data to be size stride * height.
(we should probably add some checks to parent side to ensure that)

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v2

This adds the trivial check to the parent side.

https://hg.mozilla.org/integration/mozilla-inbound/rev/18a3c813d1fb

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/18a3c813d1fb

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

I actually want to keep this open until it is clear this is actually fixed.
(I didn't manage to reproduce the crash ever)

Comment 16

[(Away)]

(In reply to Olli Pettay [:smaug] from comment #15)
> I actually want to keep this open until it is clear this is actually fixed.
> (I didn't manage to reproduce the crash ever)

It's pretty clearly fixed on crash-stats. By the way, I just noticed that it only happened on Win64 builds (unless Win32 merely got a different signature).

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Thanks

Comment 18

[Tracy Walker [:tracy]]

No crashes reported since builds with the fix from here.