Issue 575563002: Fix adopting a video element with controls

Issue 575563002: Fix adopting a video element with controls (Closed)

Created:
6 years, 3 months ago by aberent

Modified:
6 years, 3 months ago

Reviewers:
Peter Beverloo, philipj_slow

CC:
blink-reviews, feature-media-reviews_chromium.org, philipj_slow, eric.carlson_apple.com

Base URL:
https://chromium.googlesource.com/chromium/blink.git@master

Project:
blink

Visibility:
Public.

More Reviews

Description

Fix adopting a video element with controls This CL moves the creation of video controls from HTMLMediaElement::prepareForLoad() to HTMLMediaElement::loadInternal(). This is needed because Blink forbids synchonous dispatch of events within Node::appendChild() but creating the controls causes events to be dispatched synchronously, hence causing a crash if the controls are created within appendChild(). appendChild() calls prepareForLoad() synchronously, but calls loadInternal() asynchronously. This CL also adds a new layout test testing appending video elements with controls. BUG=414373 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182065

Patch Set 1 #

Patch Set 2 : Fix the bug #

Patch Set 3 : Fix TestExpectations and add expected test results #

Total comments: 4

Patch Set 4 : Fix philipj@'s comments #

Created: 6 years, 3 months ago

Download [raw] [tar.bz2]

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+21 lines, -2 lines)			Patch
	A	LayoutTests/media/adopt-node-video-controls-crash.html	View	1 2 3	1 chunk	+19 lines, -0 lines	0 comments	Download
	M	Source/core/html/HTMLMediaElement.cpp	View	1 2 3	1 chunk	+2 lines, -2 lines	0 comments	Download

Messages

Total messages: 11 (3 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

aberent

philipj@opera.com: Please review changes. I would be greatful for a quick review, since this is ...

6 years, 3 months ago (2014-09-16 10:26:48 UTC) #4

aberent

Email title is wrong. This is actually a fix for the problem, not just a ...

6 years, 3 months ago (2014-09-16 10:28:49 UTC) #5

philipj_slow

https://codereview.chromium.org/575563002/diff/40001/LayoutTests/media/adopt-node-video-controls-crash.html File LayoutTests/media/adopt-node-video-controls-crash.html (right): https://codereview.chromium.org/575563002/diff/40001/LayoutTests/media/adopt-node-video-controls-crash.html#newcode59 LayoutTests/media/adopt-node-video-controls-crash.html:59: <div id=parent><video controls id=vid></video></div> This test is much bigger ...

6 years, 3 months ago (2014-09-16 11:59:39 UTC) #6

https://codereview.chromium.org/575563002/diff/40001/LayoutTests/media/adopt-...
File LayoutTests/media/adopt-node-video-controls-crash.html (right):

https://codereview.chromium.org/575563002/diff/40001/LayoutTests/media/adopt-...
LayoutTests/media/adopt-node-video-controls-crash.html:59: <div id=parent><video
controls id=vid></video></div>
This test is much bigger than it needs to be. Please minimize tests to only
include the bits needed to reproduce the bu, in this case:

<!DOCTYPE html>
<title>Insert a video element with controls into another document</title>
<script src="../resources/testharness.js"></script>
<script src="../resources/testharnessreport.js"></script>
<div id="log"></div>
<script>
test(function()
{
    var video = document.createElement("video");
    video.controls = true;
    video.src = "data:,";

    var otherDocument = document.implementation.createHTMLDocument();
    otherDocument.body.appendChild(video);
});
</script>

https://codereview.chromium.org/575563002/diff/40001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/575563002/diff/40001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:834: configureMediaControls();
All existing tests and the one you've added pass if one just removes
configureMediaControls() above but does not add this one, i.e. this line is not
really tested.

A smaller fix is to replace configureMediaControls() with if
(hasMediaControls()) mediaControls()->reset(). AFAICT, the purpose of
configureMediaControls() in prepareForLoad() is to update the controls since the
currentTime, duration and other things may have changed, and if there are no
controls there's no point in creating them at this point I believe. When moving
to a new document, HTMLMediaElement::didNotifySubtreeInsertionsToDocument() is
going to call configureMediaControls() and otherwise parseAttribute() should do
it.

I suggest the more conservative fix, along with a test that would fail if the
code were removed entirely, to prove that it's actually needed there.

philipj_slow

LGTM to land with the mediaControls()->reset() fix even without an additional test as discussed on ...

6 years, 3 months ago (2014-09-16 13:58:44 UTC) #7

aberent

6 years, 3 months ago (2014-09-16 14:45:40 UTC) #8

https://codereview.chromium.org/575563002/diff/40001/LayoutTests/media/adopt-...
File LayoutTests/media/adopt-node-video-controls-crash.html (right):

https://codereview.chromium.org/575563002/diff/40001/LayoutTests/media/adopt-...
LayoutTests/media/adopt-node-video-controls-crash.html:59: <div id=parent><video
controls id=vid></video></div>
On 2014/09/16 11:59:39, philipj wrote:
> This test is much bigger than it needs to be. Please minimize tests to only
> include the bits needed to reproduce the bu, in this case:
> 
> <!DOCTYPE html>
> <title>Insert a video element with controls into another document</title>
> <script src="../resources/testharness.js"></script>
> <script src="../resources/testharnessreport.js"></script>
> <div id="log"></div>
> <script>
> test(function()
> {
>     var video = document.createElement("video");
>     video.controls = true;
>     video.src = "data:,";
> 
>     var otherDocument = document.implementation.createHTMLDocument();
>     otherDocument.body.appendChild(video);
> });
> </script>

Done.

https://codereview.chromium.org/575563002/diff/40001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/575563002/diff/40001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:834: configureMediaControls();
On 2014/09/16 11:59:39, philipj wrote:
> All existing tests and the one you've added pass if one just removes
> configureMediaControls() above but does not add this one, i.e. this line is
not
> really tested.
> 
> A smaller fix is to replace configureMediaControls() with if
> (hasMediaControls()) mediaControls()->reset(). AFAICT, the purpose of
> configureMediaControls() in prepareForLoad() is to update the controls since
the
> currentTime, duration and other things may have changed, and if there are no
> controls there's no point in creating them at this point I believe. When
moving
> to a new document, HTMLMediaElement::didNotifySubtreeInsertionsToDocument() is
> going to call configureMediaControls() and otherwise parseAttribute() should
do
> it.
> 
> I suggest the more conservative fix, along with a test that would fail if the
> code were removed entirely, to prove that it's actually needed there.
Having failed to create a test in a bit more than your suggested 30 minutes I
will go for the conservative fix without the test.

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/575563002/60001

6 years, 3 months ago (2014-09-16 14:46:28 UTC) #10

commit-bot: I haz the power

6 years, 3 months ago (2014-09-16 16:20:29 UTC) #11

Message was sent while issue was closed.

Committed patchset #4 (id:60001) as 182065

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages