Handle resource fetch failure in TextTrackLoader::load

Handle resource fetch failure in TextTrackLoader::load

It's not clear when this can happen, even after looking at
ResourceFetcher::requestResource I was unable to produce a test
case which would hit this code path. However, given that we are
null-checking already, we might as well handle it correctly.

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

[Nate Chapin, 2013-10-30 20:01:42]

On 2013/10/30 19:58:55, philipj wrote:

Have you tried loading an http:// text track from a file:// document? I would
have thought that would do it.

[philipj_slow, 2013-10-30 20:01:51]

https://codereview.chromium.org/53213002/diff/1/Source/core/loader/TextTrackL...
File Source/core/loader/TextTrackLoader.cpp (right):

https://codereview.chromium.org/53213002/diff/1/Source/core/loader/TextTrackL...
Source/core/loader/TextTrackLoader.cpp:144: m_resource->addClient(this);
Too many blank lines around here? http://www.chromium.org/blink/coding-style
doesn't give any clear answers.

[Nate Chapin, 2013-10-30 20:06:09]

https://codereview.chromium.org/53213002/diff/1/Source/core/loader/TextTrackL...
File Source/core/loader/TextTrackLoader.cpp (right):

https://codereview.chromium.org/53213002/diff/1/Source/core/loader/TextTrackL...
Source/core/loader/TextTrackLoader.cpp:144: m_resource->addClient(this);
On 2013/10/30 20:01:51, philipj wrote:
> Too many blank lines around here? http://www.chromium.org/blink/coding-style
> doesn't give any clear answers.

The rules on blank lines are pretty loose. I'd lean against a blank line there,
but if it were me, I would probably have written everything from:
    ResourceFetcher* fetcher = m_document.fetcher();
to:
    return true;

...without blank lines, on the logic that they're all part of the same concept
of actually starting the load. I tend to underuse blank lines :)

[philipj_slow, 2013-10-30 20:13:57]

https://codereview.chromium.org/53213002/diff/1/Source/core/loader/TextTrackL...
File Source/core/loader/TextTrackLoader.cpp (right):

https://codereview.chromium.org/53213002/diff/1/Source/core/loader/TextTrackL...
Source/core/loader/TextTrackLoader.cpp:144: m_resource->addClient(this);
On 2013/10/30 20:06:10, Nate Chapin wrote:
> On 2013/10/30 20:01:51, philipj wrote:
> > Too many blank lines around here? http://www.chromium.org/blink/coding-style
> > doesn't give any clear answers.
> 
> The rules on blank lines are pretty loose. I'd lean against a blank line
there,
> but if it were me, I would probably have written everything from:
>     ResourceFetcher* fetcher = m_document.fetcher();
> to:
>     return true;
> 
> ...without blank lines, on the logic that they're all part of the same concept
> of actually starting the load. I tend to underuse blank lines :)

I've always wanted to be like you, so I'll go with that :)

[Nate Chapin, 2013-10-30 20:21:04]

LGTM, so long as you feel you've done your due diligence in trying to trigger
the null m_resource codepath.

[philipj_slow, 2013-10-30 20:28:28]

On 2013/10/30 20:21:04, Nate Chapin wrote:
> LGTM, so long as you feel you've done your due diligence in trying to trigger
> the null m_resource codepath.

[philipj_slow, 2013-10-30 20:34:19]

On 2013/10/30 20:28:28, philipj wrote:
> On 2013/10/30 20:21:04, Nate Chapin wrote:
> > LGTM, so long as you feel you've done your due diligence in trying to
trigger
> > the null m_resource codepath.

The only check in ResourceFetcher::requestResource that I could see how to
influence is url.isValid(), but I was unable to trigger it with a URL like
"philip://bla". I should try a bit more to sneak an invalid URL past
HTMLTrackElement::canLoadUrl to hit this condition tomorrow before I commit
this, but if I fail I'll just commit and forever wonder...

[Nate Chapin, 2013-10-30 20:38:08]

On 2013/10/30 20:34:19, philipj wrote:
> On 2013/10/30 20:28:28, philipj wrote:
> > On 2013/10/30 20:21:04, Nate Chapin wrote:
> > > LGTM, so long as you feel you've done your due diligence in trying to
> trigger
> > > the null m_resource codepath.
> 
> The only check in ResourceFetcher::requestResource that I could see how to
> influence is url.isValid(), but I was unable to trigger it with a URL like
> "philip://bla". I should try a bit more to sneak an invalid URL past
> HTMLTrackElement::canLoadUrl to hit this condition tomorrow before I commit
> this, but if I fail I'll just commit and forever wonder...

It's quite possible that canLoadUrl() and its CSP enforcement provide stricter
defenses than the checks in ResourceFetcher::canRequest().

[philipj_slow, 2013-10-31 07:58:02]

I've tried for another 20 minutes to come up with some input for which
getNonEmptyURLAttribute() would return a non-valid string, but none of "", ":"
and "�" (a single U+FFFD) do the trick.

It might be easier to cover this with a unittest, but TextTrackLoader might have
to work interfaces instead of the concrete WebVTTParser, RawResource, Document
members.

Checking in and hoping for a better future.

[commit-bot: I haz the power, 2013-10-31 07:58:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/philipj@opera.com/53213002/80001

[commit-bot: I haz the power, 2013-10-31 09:23:30]

Change committed as 161040