Block load event dispatching on old document when an HTMLMediaElement is moved between documents.

Block load event dispatching on old document when an HTMLMediaElement is moved between documents.

BUG=272786
TEST=LayoutTests/http/tests/media/video-in-iframe-crash.html

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

[abarth-chromium, 2013-10-01 19:49:18]

https://codereview.chromium.org/25362002/diff/7001/LayoutTests/http/tests/med...
File LayoutTests/http/tests/media/video-in-iframe-crash.html (right):

https://codereview.chromium.org/25362002/diff/7001/LayoutTests/http/tests/med...
LayoutTests/http/tests/media/video-in-iframe-crash.html:20: //consoleWrite("Load
" + loadCount);
Please remove commented out code.

https://codereview.chromium.org/25362002/diff/7001/LayoutTests/http/tests/med...
LayoutTests/http/tests/media/video-in-iframe-crash.html:34:
window.setTimeout(reloadPage, Math.random() * 100);
Please don't call Math.random() in a test.  Just pick a number.

How long does this test take to run?  Is there no way of making a deterministic
test?

https://codereview.chromium.org/25362002/diff/7001/LayoutTests/http/tests/med...
LayoutTests/http/tests/media/video-in-iframe-crash.html:39: iframe.src =
"../../media-resources/resources/frame_size_change.webm";
This is new to me, but I assume this works..

https://codereview.chromium.org/25362002/diff/7001/Source/core/html/HTMLMedia...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/25362002/diff/7001/Source/core/html/HTMLMedia...
Source/core/html/HTMLMediaElement.cpp:396: // and there is no risk of
dispatching a load event from within the destructor.
This code looks very fragile.  Is there a better design that's less fragile?

[acolwell GONE FROM CHROMIUM, 2013-10-01 22:02:03]

https://codereview.chromium.org/25362002/diff/7001/LayoutTests/http/tests/med...
File LayoutTests/http/tests/media/video-in-iframe-crash.html (right):

https://codereview.chromium.org/25362002/diff/7001/LayoutTests/http/tests/med...
LayoutTests/http/tests/media/video-in-iframe-crash.html:20: //consoleWrite("Load
" + loadCount);
On 2013/10/01 19:49:19, abarth wrote:
> Please remove commented out code.

Done.

https://codereview.chromium.org/25362002/diff/7001/LayoutTests/http/tests/med...
LayoutTests/http/tests/media/video-in-iframe-crash.html:34:
window.setTimeout(reloadPage, Math.random() * 100);
On 2013/10/01 19:49:19, abarth wrote:
> Please don't call Math.random() in a test.  Just pick a number.
Done. This test was based on the ClusterFuzz test that was able to reliably
repro the crash.

> 
> How long does this test take to run? 
It takes about a second.

> Is there no way of making a deterministic test?
The problem is that we have to catch the iframe load before it has reached the
completed state but after it has initiated the resource load in the video tag.
If you have other tricks that can help me accomplish this, I'd be happy to try
them. There is a dance here between the renderer thread and the media thread,
that I believe makes this difficult to reproduce deterministically.

https://codereview.chromium.org/25362002/diff/7001/LayoutTests/http/tests/med...
LayoutTests/http/tests/media/video-in-iframe-crash.html:39: iframe.src =
"../../media-resources/resources/frame_size_change.webm";
On 2013/10/01 19:49:19, abarth wrote:
> This is new to me, but I assume this works..
It's how we include stuff from the media/ directory so we don't have to
duplicate code. You can see this in the other tests and script tag above.

https://codereview.chromium.org/25362002/diff/7001/Source/core/html/HTMLMedia...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/25362002/diff/7001/Source/core/html/HTMLMedia...
Source/core/html/HTMLMediaElement.cpp:396: // and there is no risk of
dispatching a load event from within the destructor.
On 2013/10/01 19:49:19, abarth wrote:
> This code looks very fragile.  Is there a better design that's less fragile?
This increment/decrementLoadEventDelayCount() does seem fragile, but I'm not
sure how else to handle it. I'm essentially doing the same thing that is being
done in the HTMLMediaElement destructor to avoid event dispatch from within
m_player's destructor. The tricky part here though is that I can't use
document() because m_player is still associated with the old document. 

I'm also going to need to merge this fix to multiple branches so I'm trying to
keep the changes minimal.(see bug details) If a better solution requires more
invasive changes, I'd prefer to defer that to a followup CL instead to avoid
merge pain.

[abarth-chromium, 2013-10-01 22:05:16]

Would a WebFrameTest give you better control over the timing and let you make
the test deterministic?

[acolwell GONE FROM CHROMIUM, 2013-10-02 01:01:48]

On 2013/10/01 22:05:16, abarth wrote:
> Would a WebFrameTest give you better control over the timing and let you make
> the test deterministic?
I've never heard of that. I'll investigate.

[acolwell GONE FROM CHROMIUM, 2013-10-04 20:23:18]

On 2013/10/02 01:01:48, acolwell wrote:
> On 2013/10/01 22:05:16, abarth wrote:
> > Would a WebFrameTest give you better control over the timing and let you
make
> > the test deterministic?
> I've never heard of that. I'll investigate.

I'm still having a heck of a time making a more deterministic test. I have
removed the setTimeout() call for the page reload which makes the LayoutTest a
little more deterministic. I haven't been able to remove the other setTimeout()
call yet. I'm still looking into using a WebFrameTest. I'd really like to land
the HTMLMediaElement changes since this is a high priority fix that effects
multiple branches. I'm happy to followup with a better test if I can, but this
is the best I have got right now.

[abarth-chromium, 2013-10-05 05:53:34]

lgtm

Ok.  LGTM.  I suspect this test will just end up being flaky and be
disabled/deleted.  Would you be willing to check
http://test-results.appspot.com/dashboards/flakiness_dashboard.html in a week or
so to see how well this test is working?

[commit-bot: I haz the power, 2013-10-05 05:53:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/acolwell@chromium.org/25362002/22001

[commit-bot: I haz the power, 2013-10-05 08:26:59]

Retried try job too often on mac_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_blink_...

[acolwell GONE FROM CHROMIUM, 2013-10-07 15:34:52]

On 2013/10/05 05:53:34, abarth wrote:
> lgtm
> 
> Ok.  LGTM.  I suspect this test will just end up being flaky and be
> disabled/deleted.  Would you be willing to check
> http://test-results.appspot.com/dashboards/flakiness_dashboard.html in a week
or
> so to see how well this test is working?

Thank you. I will definitely monitor its flakiness. Hopefully I can also find a
better way to test this soon.

[commit-bot: I haz the power, 2013-10-07 15:35:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/acolwell@chromium.org/25362002/22001

[commit-bot: I haz the power, 2013-10-07 16:27:28]

Change committed as 159031

[eric.carlson, 2013-11-07 17:00:00]

On 2013/10/04 20:23:18, acolwell wrote:
> 
> I'm still having a heck of a time making a more deterministic test. I have
> removed the setTimeout() call for the page reload which makes the LayoutTest a
> little more deterministic. I haven't been able to remove the other
setTimeout()
> call yet. I'm still looking into using a WebFrameTest. I'd really like to land
> the HTMLMediaElement changes since this is a high priority fix that effects
> multiple branches. I'm happy to followup with a better test if I can, but this
> is the best I have got right now.

FWIW, I replaced the timeout with "iframe.onload =
moveIframeBodyIntoDocumentBody" when I merged these changes to WebKit.

[eric.carlson, 2013-11-07 17:44:34]

On 2013/11/07 17:00:00, eric.carlson wrote:
> 
> FWIW, I replaced the timeout with "iframe.onload =
> moveIframeBodyIntoDocumentBody" when I merged these changes to WebKit.

Or rather when I looked into merging the changes. It turns out this change isn't
necessary in WebKit.

[acolwell GONE FROM CHROMIUM, 2013-11-07 17:58:13]

On 2013/11/07 17:44:34, eric.carlson wrote:
> On 2013/11/07 17:00:00, eric.carlson wrote:
> > 
> > FWIW, I replaced the timeout with "iframe.onload =
> > moveIframeBodyIntoDocumentBody" when I merged these changes to WebKit.
> 
> Or rather when I looked into merging the changes. It turns out this change
isn't
> necessary in WebKit.

I tried using onload when I was working on this bug. Doing that changed the
behavior enough to prevent the crash from reproducing.