[WIP] Introduce ScriptResourceData

Introduce ScriptResourceData

Previously, ClassicPendingScript::GetSource(), ScriptSourceCode, and
related methods refer ScriptResource directly when e.g. getting the
script source text on execution.
However, ScriptResource::GetStatus(), SourceText() etc. can be reset
if revalidation occurs, causing DCHECK() failures and potentially
script execution failure.

This CL introduces ScriptResourceData, the abstraction of everything
accessed on or after NotifyFinished(), i.e. a kind of "result of
script loading".
A user of ScriptResource gets a reference to ScriptResourceData
on NotifyFinished(), saves the reference and accesses
ScriptResourceData instead of ScriptResource after NotifyFinished().
The reference to ScriptResourceData is not reset due to revalidation
and thus always available on or after NotifyFinished().

This CL also adds unit tests of ClassicPendingScript that asserts
ClassicPendingScript::GetSource() always returns the source text 
of the original response, regardless of revalidation.

BUG=692856, 694702

[hiroshige, 2017-04-28 23:29:26]

Description was changed from

==========
Introduce ScriptResourceData

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

to

==========
Introduce ScriptResourceData

BUG=692856
==========

[hiroshige, 2017-04-29 00:00:55]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-29 00:01:27]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-04-29 00:27:51]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-29 00:28:34]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-29 01:38:34]

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

[commit-bot: I haz the power, 2017-04-29 01:38:35]

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

[hiroshige, 2017-05-01 19:32:58]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-01 19:33:38]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-05-01 19:48:45]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-01 19:49:37]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-05-01 20:22:56]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-01 20:23:26]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-05-01 20:28:58]

Description was changed from

==========
Introduce ScriptResourceData

BUG=692856
==========

to

==========
Introduce ScriptResourceData

Previously, ClassicPendingScript::GetSource(), ScriptSourceCode, and
related methods refer ScriptResource directly when e.g. getting the
script source text on execution.
However, ScriptResource::GetStatus(), SourceText() etc. can be reset
if revalidation occurs, causing DCHECK() failures and potentially
script execution failure.

This CL makes ScriptResource to save the necessary data for script
compilation and execution as ScriptResourceData when loading is
finished, and makes ClassicPendingScript, ScriptSourceCode and bindings
code to use the saved ScriptResourceData that is not reset after
NotifyFinished().

This CL also adds unit tests of ClassicPendingScript that asserts
ClassicPendingScript::GetSource() always returns the source text 
of the original response, regardless of revalidation.

BUG=692856, 694702
==========

[hiroshige, 2017-05-01 20:28:58]

hiroshige@chromium.org changed reviewers:
+ japhet@chromium.org, kouhei@chromium.org

[hiroshige, 2017-05-01 20:29:51]

hiroshige@chromium.org changed reviewers:
+ landell@opera.com, yhirano@chromium.org

[hiroshige, 2017-05-01 20:56:38]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-01 20:57:32]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-05-01 21:02:35]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-01 21:03:10]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-05-01 21:55:22]

hiroshige@chromium.org changed reviewers:
+ kinuko@chromium.org

[hiroshige, 2017-05-01 22:00:10]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-01 22:01:07]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-05-02 00:22:22]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 00:23:09]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-02 02:37:33]

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

[commit-bot: I haz the power, 2017-05-02 02:37:35]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2017-05-15 20:18:28]

PTAL

[kouhei (in TOK), 2017-05-15 20:56:32]

lgtm % class comment

https://codereview.chromium.org/2724673002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/resource/ScriptResourceData.h
(right):

https://codereview.chromium.org/2724673002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/resource/ScriptResourceData.h:19: class
CORE_EXPORT ScriptResourceData final
Please write a class comment.

[kouhei (in TOK), 2017-05-15 20:56:32]

lgtm % class comment

https://codereview.chromium.org/2724673002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/resource/ScriptResourceData.h
(right):

https://codereview.chromium.org/2724673002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/resource/ScriptResourceData.h:19: class
CORE_EXPORT ScriptResourceData final
Please write a class comment.

[hiroshige, 2017-05-15 22:31:35]

Description was changed from

==========
Introduce ScriptResourceData

Previously, ClassicPendingScript::GetSource(), ScriptSourceCode, and
related methods refer ScriptResource directly when e.g. getting the
script source text on execution.
However, ScriptResource::GetStatus(), SourceText() etc. can be reset
if revalidation occurs, causing DCHECK() failures and potentially
script execution failure.

This CL makes ScriptResource to save the necessary data for script
compilation and execution as ScriptResourceData when loading is
finished, and makes ClassicPendingScript, ScriptSourceCode and bindings
code to use the saved ScriptResourceData that is not reset after
NotifyFinished().

This CL also adds unit tests of ClassicPendingScript that asserts
ClassicPendingScript::GetSource() always returns the source text 
of the original response, regardless of revalidation.

BUG=692856, 694702
==========

to

==========
Introduce ScriptResourceData

Previously, ClassicPendingScript::GetSource(), ScriptSourceCode, and
related methods refer ScriptResource directly when e.g. getting the
script source text on execution.
However, ScriptResource::GetStatus(), SourceText() etc. can be reset
if revalidation occurs, causing DCHECK() failures and potentially
script execution failure.

This CL introduces ScriptResourceData, the abstraction of everything
accessed on or after NotifyFinished(), i.e. a kind of "result of
script loading".
A user of ScriptResource gets a reference to ScriptResourceData
on NotifyFinished(), saves the reference and accesses
ScriptResourceData instead of ScriptResource after NotifyFinished().
The reference to ScriptResourceData is not reset due to revalidation
and thus always available on or after NotifyFinished().

This CL also adds unit tests of ClassicPendingScript that asserts
ClassicPendingScript::GetSource() always returns the source text 
of the original response, regardless of revalidation.

BUG=692856, 694702
==========

[hiroshige, 2017-05-15 23:53:37]

Two issues (offline discussion w/ kouhei@):

1.
ResourceResponse is copied from Resource to ScriptResourceData.
Doesn't this cause performance regressions?

I might want to make ResourceResponse RefCounted or
GarbageCollected to make Resource and ScriptResourceData share
a ResourceResource without copying. This is because similar
FooResourceData patterns might be applicable to other resources
(particularly ImageResource) and I feel it would be convenient if we
can share ResourceResponse.
But this would be a large change...

2.
Is it safe to access CachedMetadataHandler after NotifyFinished() because it
causes modification?
Also, having non-const reference in ScriptResourceData looks
breaking the concept of ScriptResourceData as 'a simple result
of ScriptResource loading'.

[kinuko, 2017-05-18 05:43:30]

Works for me / looking reasonable.  For perf issue we might want to land to see
if it actually regresses something?  We might want to change how we carry
ResourceResponse around but it feels it'd need separate discussion.

Reg: CacheHandler-- do you mean the current code's not really guaranteeing the
constness due to that?

https://codereview.chromium.org/2724673002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptSourceCode.h (right):

https://codereview.chromium.org/2724673002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptSourceCode.h:68: const
ScriptResourceData* GetResource() const { return resource_; }
GetResourceData() ?

https://codereview.chromium.org/2724673002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/2724673002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp:407: const
ScriptResourceData* resource,
resource_data ?

https://codereview.chromium.org/2724673002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp:462: const
ScriptResourceData* resource,
ditto

https://codereview.chromium.org/2724673002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ClassicScript.cpp (right):

https://codereview.chromium.org/2724673002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ClassicScript.cpp:20: const
ScriptResourceData* resource,
ditto

https://codereview.chromium.org/2724673002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/resource/ScriptResourceData.h
(right):

https://codereview.chromium.org/2724673002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/resource/ScriptResourceData.h:19: class
CORE_EXPORT ScriptResourceData final
On 2017/05/15 20:56:32, kouhei (back in TOK) wrote:
> Please write a class comment.

+1, also would be nice to explicitly note that this is immutable thing.

[yhirano, 2017-05-29 09:05:52]

Sorry, I didn't notice this was ready for review. Please remove "WIP" from the
issue title. 

Would it be better to make the getter available only during CheckNotify? I think
the data is conceptually a parameter of NotifyFinished.

[kouhei (in TOK), 2017-05-29 09:11:54]

I think the CL is looking good enough to proceed w/ TODO(hiroshige) comments for
the caveats (e.g. CachedMetadataHandler)

On 2017/05/29 09:05:52, yhirano wrote:
> Sorry, I didn't notice this was ready for review. Please remove "WIP" from the
> issue title. 
> 
> Would it be better to make the getter available only during CheckNotify? I
think
> the data is conceptually a parameter of NotifyFinished.

+1