Make Resource RefCountedWillBeGarbageCollectedFinalized, attempt #2

Make Resource RefCountedWillBeGarbageCollectedFinalized, attempt #2

Resource's lifetime is currently managed by an unholy mix of
factors that would ideally be represented in a more canonical
way. Historically, some of this was necessary, and it was
glossed over because of ResourcePtr which, in addition to
keeping the Resource alive, performed a key part of
revalidation handling in the case of a 304. Now that
ResourcePtr's revalidation behavior has been removed,
standardize Resource's lifetime management.

BUG=571190
Committed: https://crrev.com/486e98700f552d7a5c4fbc1b3a9b5e9f2a6cf934
Cr-Commit-Position: refs/heads/master@{#374220}

[Nate Chapin, 2016-02-04 23:13:36]

Description was changed from

==========
Make Resource RefCountedWillBeGarbageCollectedFinalized, attempt #2

BUG=
==========

to

==========
Make Resource RefCountedWillBeGarbageCollectedFinalized, attempt #2

Resource's lifetime is currently managed by an unholy mix of
factors that would ideally be represented in a more canonical
way. Historically, some of this was necessary, and it was
glossed over because of ResourcePtr which, in addition to
keeping the Resource alive, performed a key part of
revalidation handling in the case of a 304. Now that
ResourcePtr's revalidation behavior has been removed,
standardize Resource's lifetime management.

BUG=571190
==========

[Nate Chapin, 2016-02-04 23:24:37]

japhet@chromium.org changed reviewers:
+ haraken@chromium.org

[Nate Chapin, 2016-02-04 23:24:38]

Let's try this again! :)

To see the bug fixes, diff between Patch Sets 1 and 2. One bug fix is on
CSSFontFaceSrcValue.cpp, and the other is all the rest of the diffs.

https://codereview.chromium.org/1667843003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp (right):

https://codereview.chromium.org/1667843003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp:96:
RefPtrWillBeRawPtr<FontResource> resource = FontResource::fetch(request,
document->fetcher());
This fixes https://code.google.com/p/chromium/issues/detail?id=583609. Users of
m_fetched expect its resource() to be non-null.

https://codereview.chromium.org/1667843003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/PendingScript.cpp (right):

https://codereview.chromium.org/1667843003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/PendingScript.cpp:57: void
PendingScript::dispose()
Moving the guts of ScriptLoader::detach here fixes
https://code.google.com/p/chromium/issues/detail?id=583624 (which is probably
the root cause of https://code.google.com/p/chromium/issues/detail?id=583630
too).

https://code.google.com/p/chromium/issues/detail?id=583624#c5 describes the
pre-finalizer ordering problem that causes the crash. This guarantees the old
ordering: ScriptLoader has its client removed before the superclass
ResourceOwner's pre-finalizer runs, which will remove this as a client and clear
resource().

[haraken, 2016-02-04 23:56:30]

https://codereview.chromium.org/1667843003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp (right):

https://codereview.chromium.org/1667843003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp:104: return
m_fetched->resource();

With the above fix, m_fetched can be null here. Isn't it problematic?

https://codereview.chromium.org/1667843003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/PendingScript.cpp (right):

https://codereview.chromium.org/1667843003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/PendingScript.cpp:93: void
PendingScript::stopWatchingForLoad(ScriptResourceClient* client)

You won't need to pass |client| since PendingScript already knows m_client.

https://codereview.chromium.org/1667843003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/PendingScript.h (right):

https://codereview.chromium.org/1667843003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/PendingScript.h:93: ScriptResourceClient*
m_client;

In long term, we want to move ResourceClients to Oilpan's heap and use a Member
here.

https://codereview.chromium.org/1667843003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ScriptLoader.cpp (right):

https://codereview.chromium.org/1667843003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ScriptLoader.cpp:114: void
ScriptLoader::detach()

Who calls ScriptLoader::detach() after this CL? Maybe can we remove this
entirely?

[Nate Chapin, 2016-02-06 00:34:07]

https://codereview.chromium.org/1667843003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp (right):

https://codereview.chromium.org/1667843003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp:104: return
m_fetched->resource();
On 2016/02/04 23:56:30, haraken wrote:
> 
> With the above fix, m_fetched can be null here. Isn't it problematic?

Yep. Fixed.

https://codereview.chromium.org/1667843003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/PendingScript.cpp (right):

https://codereview.chromium.org/1667843003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/PendingScript.cpp:93: void
PendingScript::stopWatchingForLoad(ScriptResourceClient* client)
On 2016/02/04 23:56:30, haraken wrote:
> 
> You won't need to pass |client| since PendingScript already knows m_client.

Agreed, removed. I could see the argument for keeping |client| and asserting
m_client == client, but that's probably not necessary.

https://codereview.chromium.org/1667843003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/PendingScript.h (right):

https://codereview.chromium.org/1667843003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/PendingScript.h:93: ScriptResourceClient*
m_client;
On 2016/02/04 23:56:30, haraken wrote:
> 
> In long term, we want to move ResourceClients to Oilpan's heap and use a
Member
> here.

Yep, someday :)

https://codereview.chromium.org/1667843003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ScriptLoader.cpp (right):

https://codereview.chromium.org/1667843003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ScriptLoader.cpp:114: void
ScriptLoader::detach()
On 2016/02/04 23:56:30, haraken wrote:
> 
> Who calls ScriptLoader::detach() after this CL? Maybe can we remove this
> entirely?

ScriptRunner::notifyScriptLoadError uses it. I think it's still needed?

[haraken, 2016-02-06 01:41:48]

LGTM to reland.

[Nate Chapin, 2016-02-08 17:26:43]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2016-02-08 17:28:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1667843003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1667843003/40001

[commit-bot: I haz the power, 2016-02-08 20:49:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-08 20:49:41]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Nate Chapin, 2016-02-08 20:52:28]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2016-02-08 20:56:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1667843003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1667843003/40001

[commit-bot: I haz the power, 2016-02-08 23:27:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-08 23:27:35]

Failed to request the patch to try. Please note that binary files are still
unsupported at the moment, this is being worked on.

Thanks for your patience.

Transient error: No JSON object could be decoded

[commit-bot: I haz the power, 2016-02-08 23:29:21]

The CQ bit was unchecked by commit-bot@chromium.org

[Nate Chapin, 2016-02-08 23:40:31]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2016-02-08 23:42:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1667843003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1667843003/40001

[commit-bot: I haz the power, 2016-02-08 23:55:56]

Description was changed from

==========
Make Resource RefCountedWillBeGarbageCollectedFinalized, attempt #2

Resource's lifetime is currently managed by an unholy mix of
factors that would ideally be represented in a more canonical
way. Historically, some of this was necessary, and it was
glossed over because of ResourcePtr which, in addition to
keeping the Resource alive, performed a key part of
revalidation handling in the case of a 304. Now that
ResourcePtr's revalidation behavior has been removed,
standardize Resource's lifetime management.

BUG=571190
==========

to

==========
Make Resource RefCountedWillBeGarbageCollectedFinalized, attempt #2

Resource's lifetime is currently managed by an unholy mix of
factors that would ideally be represented in a more canonical
way. Historically, some of this was necessary, and it was
glossed over because of ResourcePtr which, in addition to
keeping the Resource alive, performed a key part of
revalidation handling in the case of a 304. Now that
ResourcePtr's revalidation behavior has been removed,
standardize Resource's lifetime management.

BUG=571190
==========

[commit-bot: I haz the power, 2016-02-08 23:55:57]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-02-08 23:57:11]

Description was changed from

==========
Make Resource RefCountedWillBeGarbageCollectedFinalized, attempt #2

Resource's lifetime is currently managed by an unholy mix of
factors that would ideally be represented in a more canonical
way. Historically, some of this was necessary, and it was
glossed over because of ResourcePtr which, in addition to
keeping the Resource alive, performed a key part of
revalidation handling in the case of a 304. Now that
ResourcePtr's revalidation behavior has been removed,
standardize Resource's lifetime management.

BUG=571190
==========

to

==========
Make Resource RefCountedWillBeGarbageCollectedFinalized, attempt #2

Resource's lifetime is currently managed by an unholy mix of
factors that would ideally be represented in a more canonical
way. Historically, some of this was necessary, and it was
glossed over because of ResourcePtr which, in addition to
keeping the Resource alive, performed a key part of
revalidation handling in the case of a 304. Now that
ResourcePtr's revalidation behavior has been removed,
standardize Resource's lifetime management.

BUG=571190

Committed: https://crrev.com/486e98700f552d7a5c4fbc1b3a9b5e9f2a6cf934
Cr-Commit-Position: refs/heads/master@{#374220}
==========

[commit-bot: I haz the power, 2016-02-08 23:57:13]

Patchset 3 (id:??) landed as
https://crrev.com/486e98700f552d7a5c4fbc1b3a9b5e9f2a6cf934
Cr-Commit-Position: refs/heads/master@{#374220}

[please use gerrit instead, 2016-02-09 00:37:57]

On 2016/02/08 23:57:13, commit-bot: I haz the power wrote:
> Patchset 3 (id:??) landed as
> https://crrev.com/486e98700f552d7a5c4fbc1b3a9b5e9f2a6cf934
> Cr-Commit-Position: refs/heads/master@{#374220}

[please use gerrit instead, 2016-02-09 00:41:07]

Please fix this failure, if it's caused by your patch:

Regressions: Unexpected text-only failures (1)
  css3/fonts/font-style-matching-5.html [ Failure ]

https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Trusty...

[Nate Chapin, 2016-02-09 19:49:57]

On 2016/02/09 00:41:07, Rouslan wrote:
> Please fix this failure, if it's caused by your patch:
> 
> Regressions: Unexpected text-only failures (1)
>   css3/fonts/font-style-matching-5.html [ Failure ]
> 
>
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Trusty...

Based on
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=css...,
it looks like this test has been flaky for the last few days. I don't see any
particular evidence that this patch made it more flaky. Did I miss something?