Fix a document memory leak in drag-and-drop.

Fix a document memory leak in drag-and-drop.

TEST=fast/events/drag-leak-document.html (run_webkit_tests with --enable-leak-detection)
BUG=364392
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175257

[Dai Mikurube (NOT FULLTIME), 2014-05-30 12:26:05]

Trying to fix a memory leak crbug.com/364392. Not sure it's the right fix,
though.

Could you take a brief look? Not for commit immediately. It looks like fixing
the following LayoutTests.

compositing/drag-opacity-crash.html
editing/pasteboard/can-read-in-dragstart-event.html
editing/pasteboard/drag-prioritizes-draggable-container-over-image.html
editing/selection/drag-start-event-client-x-y.html
editing/selection/user-drag-element-and-user-select-none.html
fast/css/user-drag-none.html
fast/css-generated-content/drag-state.html
fast/events/bogus-dropEffect-effectAllowed.html
fast/events/clear-drag-state.html
fast/events/drag-and-drop-autoscroll-frameset.html
fast/events/drag-and-drop-autoscroll-inner-frame.html
fast/events/drag-and-drop-autoscroll-mainframe.html
fast/events/drag-and-drop-autoscroll.html
fast/events/drag-and-drop-dataTransfer-types-nocrash.html
fast/events/drag-and-drop-fire-drag-dragover.html
fast/events/drag-and-drop-set-drag-data-arguments.html
fast/events/drag-and-drop.html
fast/events/drag-customData.html
fast/events/drag-dataTransferItemList-file-handling.html
fast/events/drag-dataTransferItemList.html
fast/events/drag-display-none-element.html
fast/events/drag-downloadURL.html
fast/events/drag-svg-image-crash.html
fast/events/dragging-mouse-moves.html
fast/events/drop-generate-user-gesture.html
fast/events/dropzone-001.html
fast/events/touch/gesture/long-press-drag-drop-touch-editing-combined-in-iframe.html
fast/events/touch/gesture/long-press-on-draggable-element-triggers-drag.html
fast/repaint/japanese-rl-selection-clear.html
fast/text/international/hebrew-selection.html
virtual/softwarecompositing/drag-opacity-crash.html

[kouhei (in TOK), 2014-05-30 12:29:54]

lgtm if the test pass.

https://codereview.chromium.org/302133002/diff/1/Source/core/page/EventHandle...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/302133002/diff/1/Source/core/page/EventHandle...
Source/core/page/EventHandler.cpp:3059: dragState().m_dragSrc = nullptr;
remove this and other m_dragSrc clear after freeClipboard()

[haraken, 2014-05-30 12:35:29]

Would it be possible to add a test?

c.f., NodeIterator-leak-document.html

https://codereview.chromium.org/302133002/diff/1/Source/core/page/EventHandle...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/302133002/diff/1/Source/core/page/EventHandle...
Source/core/page/EventHandler.cpp:3041: dragState().m_dragSrc = nullptr;

You'll need to move this to outside of the if branch. Then you can remove
"m_dragSrc = nullptr" after freeClipboard(), as kouhei-san suggested.

[Dai Mikurube (NOT FULLTIME), 2014-05-30 12:38:28]

Hmm, actually |dragState().m_dragSrc = nullptr;| caused a crush maybe because of
freeClipboard(); in tryStartDrag() which is not followed by
|dragState().m_dragSrc = nullptr;|.

[kouhei (in TOK), 2014-05-30 12:39:37]

On 2014/05/30 12:35:29, haraken wrote:
> Would it be possible to add a test?
> 
> c.f., NodeIterator-leak-document.html
You can easily make one using leak-check.js
https://code.google.com/p/chromium/codesearch#search/&q=leak-check.js&sq=pack...

However, I'm not sure if it would make sense as we already have leak detector in
waterfall.

[haraken, 2014-05-30 12:41:22]

On 2014/05/30 12:39:37, kouhei wrote:
> On 2014/05/30 12:35:29, haraken wrote:
> > Would it be possible to add a test?
> > 
> > c.f., NodeIterator-leak-document.html
> You can easily make one using leak-check.js
>
https://code.google.com/p/chromium/codesearch#search/&q=leak-check.js&sq=pack...
> 
> However, I'm not sure if it would make sense as we already have leak detector
in
> waterfall.

That's good point. Probably we won't need it.

[Dai Mikurube (NOT FULLTIME), 2014-05-30 13:24:48]

Tried adding a test case. No expectation, though. (Failing to add -expect.txt.
Will try Monday.)

[kouhei (in TOK), 2014-05-30 13:27:50]

https://codereview.chromium.org/302133002/diff/40001/LayoutTests/fast/events/...
File LayoutTests/fast/events/drag-leak-document.html (right):

https://codereview.chromium.org/302133002/diff/40001/LayoutTests/fast/events/...
LayoutTests/fast/events/drag-leak-document.html:26: </body>
<p>PASS if no leak is detected by the leak detector.</p>

[Dai Mikurube (NOT FULLTIME), 2014-06-02 02:16:14]

It's now ready. PTAL?

[haraken, 2014-06-02 02:24:16]

LGTM.

https://codereview.chromium.org/302133002/diff/60001/LayoutTests/fast/events/...
File LayoutTests/fast/events/drag-leak-document.html (right):

https://codereview.chromium.org/302133002/diff/60001/LayoutTests/fast/events/...
LayoutTests/fast/events/drag-leak-document.html:4: <script>

Nit: We normally write a <script> tag in <body>.

https://codereview.chromium.org/302133002/diff/60001/LayoutTests/fast/events/...
LayoutTests/fast/events/drag-leak-document.html:6: testRunner.waitUntilDone();

This test doesn't need to be async, so waitUntilDone is not needed.

https://codereview.chromium.org/302133002/diff/60001/LayoutTests/fast/events/...
LayoutTests/fast/events/drag-leak-document.html:18: testRunner.notifyDone();

Remove this.

[Dai Mikurube (NOT FULLTIME), 2014-06-02 03:11:50]

Done. Thanks!

https://codereview.chromium.org/302133002/diff/60001/LayoutTests/fast/events/...
File LayoutTests/fast/events/drag-leak-document.html (right):

https://codereview.chromium.org/302133002/diff/60001/LayoutTests/fast/events/...
LayoutTests/fast/events/drag-leak-document.html:4: <script>
On 2014/06/02 02:24:17, haraken wrote:
> 
> Nit: We normally write a <script> tag in <body>.

Done.

https://codereview.chromium.org/302133002/diff/60001/LayoutTests/fast/events/...
LayoutTests/fast/events/drag-leak-document.html:6: testRunner.waitUntilDone();
On 2014/06/02 02:24:17, haraken wrote:
> 
> This test doesn't need to be async, so waitUntilDone is not needed.

Done.

https://codereview.chromium.org/302133002/diff/60001/LayoutTests/fast/events/...
LayoutTests/fast/events/drag-leak-document.html:18: testRunner.notifyDone();
On 2014/06/02 02:24:17, haraken wrote:
> 
> Remove this.

Done.

[Dai Mikurube (NOT FULLTIME), 2014-06-02 06:36:06]

The CQ bit was checked by dmikurube@chromium.org

[commit-bot: I haz the power, 2014-06-02 06:36:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dmikurube@chromium.org/302133002/80001

[commit-bot: I haz the power, 2014-06-02 08:50:39]

Change committed as 175257