Window.postMessage() to self can cause document leaks

Window.postMessage() to self can cause document leaks

When a script does Window.postMessage() a PostMessageTimer object is
stored in the LocalDOMWindow until the message has been delivered, or
until it is cleared in the destructor of LocalDOMWindow. When a
LocalDOMWindow is reset the remaining messages will not be
delivered. Any PostMessageTimer objects alive at that point will be
kept until the LocalDOMWindow is destroyed. Unfortunately, the
PostMessageTimer objects keeps a reference to the source window. If
both the source and destination window of the message is the same, the
PostMessageTimer can keep that LocalDOMWindow from getting destroyed.

Removing the PostMessageTimers when stop() is invoked fixes the problem.

BUG=

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196930

[landell, 2015-06-05 11:52:22]

landell@opera.com changed reviewers:
+ ager@chromium.org, dcheng@chromium.org, sigbjorn@opera.com

[landell, 2015-06-05 11:53:01]

Reviewers: PTAL, thanks

[sof, 2015-06-05 13:01:13]

sigbjornf@opera.com changed reviewers:
+ haraken@chromium.org, sigbjornf@opera.com
- ager@chromium.org, sigbjorn@opera.com

[sof, 2015-06-05 13:01:13]

Won't the one-shot timer still fire & remove itself from that set?

[landell, 2015-06-05 13:57:24]

On 2015/06/05 13:01:13, sof wrote:
> Won't the one-shot timer still fire & remove itself from that set?

The short answer is no. blink::SuspendableTimer::stop() is called on the related
timer object when this happens.
I don't know the origin of the call to stop() but I can dig a bit more into this
if you want me to.
Do you think that it would be better if the timer fired and let the code in
LocalDOMWindow::postMessageTimerFired
handle the removal as it seems to be doing already?

[sof, 2015-06-05 14:20:25]

On 2015/06/05 13:57:24, landell wrote:
> On 2015/06/05 13:01:13, sof wrote:
> > Won't the one-shot timer still fire & remove itself from that set?
> 
> The short answer is no. blink::SuspendableTimer::stop() is called on the
related
> timer object when this happens.
> I don't know the origin of the call to stop() but I can dig a bit more into
this
> if you want me to.
> Do you think that it would be better if the timer fired and let the code in
> LocalDOMWindow::postMessageTimerFired
> handle the removal as it seems to be doing already?

I see, well spotted problem. It'll be stopped along with the other
ActiveDOMObjects, which usually happens when the Document is detached.

Overriding stop() is an alternative, along with splitting out the removal of
timers from m_postMessageTimers. That makes PostMessageTimer "more
self-contained" and is friendlier to Oilpan, where reset() isn't guaranteed to
be called.

[landell, 2015-06-08 11:27:08]

On 2015/06/05 14:20:25, sof wrote:
> 
> Overriding stop() is an alternative, along with splitting out the removal of
> timers from m_postMessageTimers. That makes PostMessageTimer "more
> self-contained" and is friendlier to Oilpan, where reset() isn't guaranteed to
> be called.

Ok, I made a new version that kills the timers on stop() instead. Was this what
you had in mind?

[sof, 2015-06-08 11:31:45]

Looks good; would it be possible to come up with a test (that the leak detector
bot would catch as leaking)?

https://codereview.chromium.org/1148133005/diff/20001/Source/core/frame/Local...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/1148133005/diff/20001/Source/core/frame/Local...
Source/core/frame/LocalDOMWindow.cpp:149: // This object is deleted now.
How about we call m_window->postMessageTimerStopped() from here & retire the
remove()s from postMessageTimerFired() ?

[landell, 2015-06-08 11:48:18]

https://codereview.chromium.org/1148133005/diff/20001/Source/core/frame/Local...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/1148133005/diff/20001/Source/core/frame/Local...
Source/core/frame/LocalDOMWindow.cpp:149: // This object is deleted now.
On 2015/06/08 11:31:45, sof wrote:
> How about we call m_window->postMessageTimerStopped() from here & retire the
> remove()s from postMessageTimerFired() ?

Sure, I think it shall be renamed to removePostMessageTimer then which will
better match the semantics.

[landell, 2015-06-08 11:51:10]

On 2015/06/08 11:31:45, sof wrote:
> Looks good; would it be possible to come up with a test (that the leak
detector
> bot would catch as leaking)?
> 

Maybe, I am not familiar how such tests are written though. Could you point me
to a similar test that I can use as a template?

[sof, 2015-06-08 13:41:22]

On 2015/06/08 11:51:10, landell wrote:
> On 2015/06/08 11:31:45, sof wrote:
> > Looks good; would it be possible to come up with a test (that the leak
> detector
> > bot would catch as leaking)?
> > 
> 
> Maybe, I am not familiar how such tests are written though. Could you point me
> to a similar test that I can use as a template?

If you've got some content that's showing up the leak of a Document, turning it
into a layout test is (hopefully) straightforward. i.e., create
fast/dom/Window/post-message-to-self-leak.html (or some such) and use
window-postmesssage-args.html in that same directory as a template. 

The leak detection bot checks that no resource leaks on shutdown, so you don't
have to check for that leak in your test somehow. You can locally test (w/
content_shell) as follows

 foo$ pwd
 ~/blink/src
 foo$ blink/tools/run_layout_tests.sh --enable-leak-detection <and whatever
other options you want> --debug 'fast/dom/Window/post-message*'
 
If the test isn't a 100% repro, that's ok.

[landell, 2015-06-10 12:50:39]

On 2015/06/08 13:41:22, sof wrote:
> If you've got some content that's showing up the leak of a Document, turning
it
> into a layout test is (hopefully) straightforward. i.e., create
> fast/dom/Window/post-message-to-self-leak.html (or some such) and use
> window-postmesssage-args.html in that same directory as a template. 
> 
> The leak detection bot checks that no resource leaks on shutdown, so you don't
> have to check for that leak in your test somehow. You can locally test (w/
> content_shell) as follows
> 
>  foo$ pwd
>  ~/blink/src
>  foo$ blink/tools/run_layout_tests.sh --enable-leak-detection <and whatever
> other options you want> --debug 'fast/dom/Window/post-message*'
>  
> If the test isn't a 100% repro, that's ok.

Added a test now that seems to work. Let's see if I've done it right.

[sof, 2015-06-10 14:37:25]

Great, thanks - leak steadily reproduces.

Could you add the expected output to the CL also? i.e.,
LayoutTests/fast/dom/Window/post-message-to-self-expected.txt

https://codereview.chromium.org/1148133005/diff/60001/LayoutTests/fast/dom/Wi...
File LayoutTests/fast/dom/Window/post-message-to-self.html (right):

https://codereview.chromium.org/1148133005/diff/60001/LayoutTests/fast/dom/Wi...
LayoutTests/fast/dom/Window/post-message-to-self.html:8: 
Add

window.jsTestIsAsync = true;

if (window.testRunner) {
    testRunner.dumpAsText();
    testRunner.waitUntilDone();
}

https://codereview.chromium.org/1148133005/diff/60001/LayoutTests/fast/dom/Wi...
LayoutTests/fast/dom/Window/post-message-to-self.html:16: testPassed("Recevied
message " + event.data);
On the next line, add a call to finishJSTest();

https://codereview.chromium.org/1148133005/diff/60001/LayoutTests/fast/dom/Wi...
LayoutTests/fast/dom/Window/post-message-to-self.html:16: testPassed("Recevied
message " + event.data);
sp: Received

[landell, 2015-06-11 07:26:05]

https://codereview.chromium.org/1148133005/diff/60001/LayoutTests/fast/dom/Wi...
File LayoutTests/fast/dom/Window/post-message-to-self.html (right):

https://codereview.chromium.org/1148133005/diff/60001/LayoutTests/fast/dom/Wi...
LayoutTests/fast/dom/Window/post-message-to-self.html:8: 
On 2015/06/10 14:37:25, sof wrote:
> Add
> 
> window.jsTestIsAsync = true;
> 
> if (window.testRunner) {
>     testRunner.dumpAsText();
>     testRunner.waitUntilDone();
> }

Done.

https://codereview.chromium.org/1148133005/diff/60001/LayoutTests/fast/dom/Wi...
LayoutTests/fast/dom/Window/post-message-to-self.html:16: testPassed("Recevied
message " + event.data);
On 2015/06/10 14:37:25, sof wrote:
> sp: Received

Done.

https://codereview.chromium.org/1148133005/diff/60001/LayoutTests/fast/dom/Wi...
LayoutTests/fast/dom/Window/post-message-to-self.html:16: testPassed("Recevied
message " + event.data);
On 2015/06/10 14:37:25, sof wrote:
> On the next line, add a call to finishJSTest();

Done.

[sof, 2015-06-11 07:30:35]

lgtm

[landell, 2015-06-11 08:18:17]

The CQ bit was checked by landell@opera.com

[landell, 2015-06-11 08:18:17]

The patchset sent to the CQ was uploaded after l-g-t-m from sigbjornf@opera.com
Link to the patchset: https://codereview.chromium.org/1148133005/#ps100001
(title: "Rebase to master")

[commit-bot: I haz the power, 2015-06-11 08:18:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1148133005/100001

[haraken, 2015-06-11 08:29:58]

LGTM

[commit-bot: I haz the power, 2015-06-11 09:16:42]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196930