Crash when calling getCueAsHTML() on a TextTrackCue with empty text

Crash when calling getCueAsHTML() on a TextTrackCue with empty text.

Even if the TextTrackCue text is empty, a document fragment should still be returned as a result value when calling getCueAsHTML(). Currently, the call crashes the browser, when the cue text is "".

TEST=Added relevant test case.
R=acolwell,jchaffraix
BUG=261653
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=155890

[vcarbune.chromium, 2013-08-05 21:56:25]

Probably this crash is present in stable/beta channels too.

[acolwell GONE FROM CHROMIUM, 2013-08-05 23:40:31]

https://codereview.chromium.org/22304002/diff/1/LayoutTests/media/track/track...
File LayoutTests/media/track/track-cue-empty-crash.html (right):

https://codereview.chromium.org/22304002/diff/1/LayoutTests/media/track/track...
LayoutTests/media/track/track-cue-empty-crash.html:12: var emptyCue = new
TextTrackCue(0, 0, "");
Should passing null also behave the same way? If so it might be nice to cover
that case as well.

https://codereview.chromium.org/22304002/diff/1/Source/core/html/track/WebVTT...
File Source/core/html/track/WebVTTParser.cpp (left):

https://codereview.chromium.org/22304002/diff/1/Source/core/html/track/WebVTT...
Source/core/html/track/WebVTTParser.cpp:332: return 0;
nit: Do you really need to jump though all the hoops below. Can't you just move
this down 2 lines and do 'return DocumentFragment::create(document);'?

[vcarbune.chromium, 2013-08-09 18:47:26]

Updated code and test case. PTAL.

https://codereview.chromium.org/22304002/diff/1/LayoutTests/media/track/track...
File LayoutTests/media/track/track-cue-empty-crash.html (right):

https://codereview.chromium.org/22304002/diff/1/LayoutTests/media/track/track...
LayoutTests/media/track/track-cue-empty-crash.html:12: var emptyCue = new
TextTrackCue(0, 0, "");
On 2013/08/05 23:40:32, acolwell wrote:
> Should passing null also behave the same way? If so it might be nice to cover
> that case as well.

null is converted to the string "null" and further processed like that - I'd say
this is appropriate, as it's the default handling in JavaScript

https://codereview.chromium.org/22304002/diff/1/Source/core/html/track/WebVTT...
File Source/core/html/track/WebVTTParser.cpp (left):

https://codereview.chromium.org/22304002/diff/1/Source/core/html/track/WebVTT...
Source/core/html/track/WebVTTParser.cpp:332: return 0;
On 2013/08/05 23:40:32, acolwell wrote:
> nit: Do you really need to jump though all the hoops below. Can't you just
move
> this down 2 lines and do 'return DocumentFragment::create(document);'?

Yes, you're right, it shouldn't go through everything below. I also gave this a
bit more thought and I think what it should actually be returned is a
DocumentFragment with an empty Text as child node (empty string). This makes it
consistent with the regular behaviour.

[acolwell GONE FROM CHROMIUM, 2013-08-09 19:19:02]

lgtm

[Julien - ping for review, 2013-08-09 20:12:12]

lgtm, usually the description should follow (loosely) what is done for git
commits with a short 1-line summary at the beginning. Here it could be (title
copied from the bug, even it is wrong and should be updated):

Crash calling TextTrackCue constructor on detached iframe window.

(then the rest of your description)

https://codereview.chromium.org/22304002/diff/5001/LayoutTests/media/track/tr...
File LayoutTests/media/track/track-cue-empty-crash.html (right):

https://codereview.chromium.org/22304002/diff/5001/LayoutTests/media/track/tr...
LayoutTests/media/track/track-cue-empty-crash.html:4: <meta
http-equiv="Content-Type" content="text/html; charset=utf-8" />
Usually meta tags are removed unless useful for the test.

[commit-bot: I haz the power, 2013-08-09 23:21:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vcarbune@chromium.org/22304002/15001

[commit-bot: I haz the power, 2013-08-10 01:06:41]

Change committed as 155890