Make ScriptLoader into a ResourceOwner

Make ScriptLoader into a ResourceOwner

This change turns ScriptLoader a ResourceOwner, that ensures that
it subscribes Resource if the owner has a reference to it, and that
it unsubscribes the Resource otherwise. Before this change,
There are some situation where ScriptLoader has a reference to Resource
but doesn't subscribe it, that is error prone and left deleted instance
being subscribed. ResourceOwner guarantees reference and subscription
consistency.

TEST=script-cancel-crash.html
BUG=368551
R=eseidel@chromium.org, japhet@chromium.org, esprehn@chromium.org

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

[Hajime Morrita, 2014-05-07 17:06:39]

@esprehn: Could you take a look? I hope this stops recent ScriptLoader crash.

[eseidel, 2014-05-07 17:23:07]

This ResourceOwner thing must be relatively new. 

This CL lgtm, but Nate is a much better reviewer for this kind of thing.

[Hajime Morrita, 2014-05-07 17:29:37]

On 2014/05/07 17:23:07, eseidel wrote:
> This ResourceOwner thing must be relatively new. 
> 
Yep. I added it a few month ago.

> This CL lgtm, but Nate is a much better reviewer for this kind of thing.
Right. Nate, could give it a glance?
I hope ScriptResource to be always notifyFinished() asynchronously. We probably
need some research to do it though.

[Nate Chapin, 2014-05-07 18:13:10]

The logic LGTM, just a couple of api questions.

https://codereview.chromium.org/271533002/diff/1/Source/core/dom/ScriptLoader...
File Source/core/dom/ScriptLoader.cpp (right):

https://codereview.chromium.org/271533002/diff/1/Source/core/dom/ScriptLoader...
Source/core/dom/ScriptLoader.cpp:409: 
Nit: Remove this line.

https://codereview.chromium.org/271533002/diff/1/Source/core/dom/ScriptLoader.h
File Source/core/dom/ScriptLoader.h (right):

https://codereview.chromium.org/271533002/diff/1/Source/core/dom/ScriptLoader...
Source/core/dom/ScriptLoader.h:49: bool
prepareScript(ResourcePtr<ScriptResource>* = 0, const TextPosition&
scriptStartPosition = TextPosition::minimumPosition(), LegacyTypeSupport =
DisallowLegacyTypeInTypeAttribute);
It looks like the return value will be false if the ResourcePtr is null and true
if it isn't. Could we just return a ResourcePtr<ScriptResource> and avoid the
pointer to a pointer?

https://codereview.chromium.org/271533002/diff/1/Source/core/html/parser/HTML...
File Source/core/html/parser/HTMLScriptRunner.h (right):

https://codereview.chromium.org/271533002/diff/1/Source/core/html/parser/HTML...
Source/core/html/parser/HTMLScriptRunner.h:80: void
requestParsingBlockingScript(const ResourcePtr<ScriptResource>&, Element*);
Like in ScriptLoader, could we return the ResourcePtr instead of taking a
reference?

[Hajime Morrita, 2014-05-07 18:27:28]

Nate, thanks for the review.
I'll fix them and land.

https://codereview.chromium.org/271533002/diff/1/Source/core/dom/ScriptLoader.h
File Source/core/dom/ScriptLoader.h (right):

https://codereview.chromium.org/271533002/diff/1/Source/core/dom/ScriptLoader...
Source/core/dom/ScriptLoader.h:49: bool
prepareScript(ResourcePtr<ScriptResource>* = 0, const TextPosition&
scriptStartPosition = TextPosition::minimumPosition(), LegacyTypeSupport =
DisallowLegacyTypeInTypeAttribute);
On 2014/05/07 18:13:10, Nate Chapin wrote:
> It looks like the return value will be false if the ResourcePtr is null and
true
> if it isn't. Could we just return a ResourcePtr<ScriptResource> and avoid the
> pointer to a pointer?
Sometimes it can be true but return null.
I agree that out-parameter is bad in general though.
Will add a small class to return.

https://codereview.chromium.org/271533002/diff/1/Source/core/html/parser/HTML...
File Source/core/html/parser/HTMLScriptRunner.h (right):

https://codereview.chromium.org/271533002/diff/1/Source/core/html/parser/HTML...
Source/core/html/parser/HTMLScriptRunner.h:80: void
requestParsingBlockingScript(const ResourcePtr<ScriptResource>&, Element*);
On 2014/05/07 18:13:10, Nate Chapin wrote:
> Like in ScriptLoader, could we return the ResourcePtr instead of taking a
> reference?
These aren't return values.

[Hajime Morrita, 2014-05-07 18:48:05]

The CQ bit was checked by morrita@chromium.org

[commit-bot: I haz the power, 2014-05-07 18:48:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/morrita@chromium.org/271533002/20001

[commit-bot: I haz the power, 2014-05-07 19:58:50]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  blink_presubmit on tryserver.blink
  linux_blink_dbg on tryserver.blink
  linux_blink_rel on tryserver.blink
  mac_blink_compile_dbg on tryserver.blink
  mac_blink_rel on tryserver.blink
  win_blink_compile_dbg on tryserver.blink
  win_blink_rel on tryserver.blink

[Hajime Morrita, 2014-05-07 20:11:07]

The CQ bit was checked by morrita@chromium.org

[commit-bot: I haz the power, 2014-05-07 20:12:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/morrita@chromium.org/271533002/40001

[commit-bot: I haz the power, 2014-05-07 21:51:29]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_blink_dbg on tryserver.blink
  linux_blink_rel on tryserver.blink
  mac_blink_rel on tryserver.blink
  win_blink_rel on tryserver.blink

[Hajime Morrita, 2014-05-07 21:55:50]

On 2014/05/07 21:51:29, I haz the power (commit-bot) wrote:
> FYI, CQ is re-trying this CL (attempt #1).
> Please consider checking whether the failures are real,
> and report flakes to mailto:chrome-troopers@google.com.
> The failing builders are:
>   linux_blink_dbg on tryserver.blink
>   linux_blink_rel on tryserver.blink
>   mac_blink_rel on tryserver.blink
>   win_blink_rel on tryserver.blink

These failures are real. I have a fix with some ugly part. 
As the branch point is approaching and this is one of top crashers, I'll land
it.
But I'll come back to cleanup then.

[Hajime Morrita, 2014-05-07 22:13:45]

The CQ bit was checked by morrita@chromium.org

[commit-bot: I haz the power, 2014-05-07 22:14:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/morrita@chromium.org/271533002/60001

[Hajime Morrita, 2014-05-07 22:18:30]

Committed patchset #4 manually as r173566 (presubmit successful).