Do not revalidate ScriptResource while it has clients [chromium/src : master]

Hiroshige Hayashizaki (Gerrit)

unread,

Aug 18, 2017, 3:57:11 AM8/18/17

to blink-...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading...@chromium.org, tyoshin...@chromium.org, Yutaka Hirano, Kouhei Ueno, Nate Chapin, Commit Bot, chromium...@chromium.org

PTAL.

Originally, I was planning to fix the issue by introducing ScriptResourceData.
But I found more timing dependencies around CachedMetadataHandler and I think handling these dependencies might take more time.
Now I'm looking for a short-term workaround. WDYT this CL?

View Change

To view, visit change 620272. To unsubscribe, or for help writing mail filters, visit settings.

Yutaka Hirano (Gerrit)

unread,

Aug 18, 2017, 7:18:00 AM8/18/17

to Hiroshige Hayashizaki, blink-...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading...@chromium.org, tyoshin...@chromium.org, Yutaka Hirano, Kouhei Ueno, Nate Chapin, Commit Bot, chromium...@chromium.org

Patch set 3:Code-Review +1

View Change

To view, visit change 620272. To unsubscribe, or for help writing mail filters, visit settings.

Nate Chapin (Gerrit)

unread,

Aug 18, 2017, 12:47:46 PM8/18/17

to Hiroshige Hayashizaki, blink-...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading...@chromium.org, tyoshin...@chromium.org, Yutaka Hirano, Kouhei Ueno, Commit Bot, chromium...@chromium.org

Hiroshige Hayashizaki (Gerrit)

unread,

Aug 18, 2017, 6:47:30 PM8/18/17

to blink-...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading...@chromium.org, tyoshin...@chromium.org, Nate Chapin, Yutaka Hirano, Kouhei Ueno, Commit Bot, chromium...@chromium.org

Patch set 3:Commit-Queue +2

View Change

To view, visit change 620272. To unsubscribe, or for help writing mail filters, visit settings.

Hiroshige Hayashizaki (Gerrit)

unread,

Aug 18, 2017, 6:56:21 PM8/18/17

to Yutaka Hirano, Nate Chapin, Kouhei Ueno, blink-...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading...@chromium.org, tyoshin...@chromium.org, chromium...@chromium.org, Commit Bot

Hiroshige Hayashizaki uploaded patch set #4 to this change.

View Change

Do not revalidate ScriptResource while it has clients

This is to suppress revalidation until ClassicPendingScript::GetSource()
to avoid DCHECK() failure there.
In the case of ClassicPendingScript, RemoveClient() is called
in ResourceOwner::SetResource(nullptr)
in ClassicPendingScript::DisposeInternal()
in PendingScript::Dispose()
in ScriptLoader::ExecuteScriptBlock() just after ClassicPendingScript::GetSource().

In other ScriptResourceClient subclasses (WorkletScriptLoader and ModuleScriptFetcher),
RemoveClient() is called in NotifyFinished(), and therefore this CL doesn't affect
the revalidation behavior so much.

Bug: 692856, 749773
Change-Id: I28cda31b8c103059fa2e05ef0ad38c9a95e11776
---
M third_party/WebKit/Source/core/loader/resource/ScriptResource.cpp
M third_party/WebKit/Source/core/loader/resource/ScriptResource.h
2 files changed, 12 insertions(+), 0 deletions(-)

To view, visit change 620272. To unsubscribe, or for help writing mail filters, visit settings.

Hiroshige Hayashizaki (Gerrit)

unread,

Aug 18, 2017, 6:56:32 PM8/18/17

to blink-...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading...@chromium.org, tyoshin...@chromium.org, Nate Chapin, Yutaka Hirano, Kouhei Ueno, Commit Bot, chromium...@chromium.org

Patch set 4:Commit-Queue +2

View Change

To view, visit change 620272. To unsubscribe, or for help writing mail filters, visit settings.

Commit Bot (Gerrit)

unread,

Aug 18, 2017, 6:57:03 PM8/18/17

to Hiroshige Hayashizaki, blink-...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading...@chromium.org, tyoshin...@chromium.org, Nate Chapin, Yutaka Hirano, Kouhei Ueno, chromium...@chromium.org

CQ is trying da patch.

Note: The patchset sent to CQ was uploaded after this CL was approved.
"Edit commit message" https://chromium-review.googlesource.com/c/620272/4

Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/620272/4

Bot data: {"action": "start", "triggered_at": "2017-08-18T22:56:29.0Z", "cq_cfg_revision": "5668c9aae8391d95373bdc16d23f5833b4e5ff37", "revision": "92ff1910fc51c79c8077cd5f7966562e3976f359"}

View Change

To view, visit change 620272. To unsubscribe, or for help writing mail filters, visit settings.

Commit Bot (Gerrit)

unread,

Aug 18, 2017, 7:03:49 PM8/18/17

to Hiroshige Hayashizaki, blink-...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading...@chromium.org, tyoshin...@chromium.org, Nate Chapin, Yutaka Hirano, Kouhei Ueno, chromium...@chromium.org

Commit Bot merged this change.

View Change

Approvals:
  Yutaka Hirano: Looks good to me
  Nate Chapin: Looks good to me
  Hiroshige Hayashizaki: Commit

Do not revalidate ScriptResource while it has clients

This is to suppress revalidation until ClassicPendingScript::GetSource()
to avoid DCHECK() failure there.
In the case of ClassicPendingScript, RemoveClient() is called
in ResourceOwner::SetResource(nullptr)
in ClassicPendingScript::DisposeInternal()
in PendingScript::Dispose()
in ScriptLoader::ExecuteScriptBlock() just after ClassicPendingScript::GetSource().

In other ScriptResourceClient subclasses (WorkletScriptLoader and ModuleScriptFetcher),
RemoveClient() is called in NotifyFinished(), and therefore this CL doesn't affect
the revalidation behavior so much.

Bug: 692856, 749773
Change-Id: I28cda31b8c103059fa2e05ef0ad38c9a95e11776

Reviewed-on: https://chromium-review.googlesource.com/620272
Commit-Queue: Hiroshige Hayashizaki <hiro...@chromium.org>
Reviewed-by: Yutaka Hirano <yhi...@chromium.org>
Reviewed-by: Nate Chapin <jap...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#495728}


---
M third_party/WebKit/Source/core/loader/resource/ScriptResource.cpp
M third_party/WebKit/Source/core/loader/resource/ScriptResource.h
2 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/third_party/WebKit/Source/core/loader/resource/ScriptResource.cpp b/third_party/WebKit/Source/core/loader/resource/ScriptResource.cpp
index e8e02cd..118e8ec 100644
--- a/third_party/WebKit/Source/core/loader/resource/ScriptResource.cpp
+++ b/third_party/WebKit/Source/core/loader/resource/ScriptResource.cpp
@@ -120,4 +120,14 @@
   return kNotSharableCrossOrigin;
 }
 
+bool ScriptResource::CanUseCacheValidator() const {
+  // Do not revalidate until ClassicPendingScript is removed, i.e. the script
+  // content is retrieved in ScriptLoader::ExecuteScriptBlock().
+  // crbug.com/692856
+  if (HasClientsOrObservers())
+    return false;
+
+  return Resource::CanUseCacheValidator();
+}
+
 }  // namespace blink
diff --git a/third_party/WebKit/Source/core/loader/resource/ScriptResource.h b/third_party/WebKit/Source/core/loader/resource/ScriptResource.h
index 3fa422f..cb0315f 100644
--- a/third_party/WebKit/Source/core/loader/resource/ScriptResource.h
+++ b/third_party/WebKit/Source/core/loader/resource/ScriptResource.h
@@ -103,6 +103,8 @@
                  const ResourceLoaderOptions&,
                  const TextResourceDecoderOptions&);
 
+  bool CanUseCacheValidator() const override;
+
   AtomicString source_text_;
 };

To view, visit change 620272. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward