New issue
Advanced search Search tips

Issue 784875 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 752711



Sign in to add a comment

Decouple CachedMetadataHandler from Resource

Project Member Reported by hirosh...@chromium.org, Nov 14 2017

Issue description

Currently CachedMetadataHandler (in fact its subclass) has a reference to Resource, but this reference should be removed:
- The existence of the reference from CMH to Resource requires that the Resource shouldn't be revalidated before the metadata are written to CachedMetadataHandler, and thus blocks Issue 752711 and V8 code cache processing after script execution.
- CachedMetadataHandler is only for metadata cache in the browser side, and shouldn't refer to Resource/MemoryCache layer.

Also, probably CachedMetadataHandler's lifetime should be fixed (CachedMetadataHandler should be created when response is received etc).
 
Summary: Decouple CachedMetadataHandler from Resource (was: Cleanup relationships between Resource and CachedMetadataHandler)
Project Member

Comment 2 by bugdroid1@chromium.org, Dec 8 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7d1a458799d108aefda87d9939c675a07c994970

commit 7d1a458799d108aefda87d9939c675a07c994970
Author: Hiroshige Hayashizaki <hiroshige@chromium.org>
Date: Fri Dec 08 00:35:25 2017

Do not use Resource in CacheOptions() in ScriptController

To remove Resource from ScriptController, this CL
- Introduces CachedMetadataHandler::IsServedFromCacheStorage(), and
- Uses it in ScriptController's CacheOptions().

Bug:  788828 ,  784875 
Change-Id: Iaf34c40ceff223357361e8a189c3bf5d33251ffc
Reviewed-on: https://chromium-review.googlesource.com/786322
Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org>
Reviewed-by: Nate Chapin <japhet@chromium.org>
Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522645}
[modify] https://crrev.com/7d1a458799d108aefda87d9939c675a07c994970/third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp
[modify] https://crrev.com/7d1a458799d108aefda87d9939c675a07c994970/third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerScriptCachedMetadataHandler.cpp
[modify] https://crrev.com/7d1a458799d108aefda87d9939c675a07c994970/third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerScriptCachedMetadataHandler.h
[modify] https://crrev.com/7d1a458799d108aefda87d9939c675a07c994970/third_party/WebKit/Source/platform/loader/fetch/CachedMetadataHandler.h
[modify] https://crrev.com/7d1a458799d108aefda87d9939c675a07c994970/third_party/WebKit/Source/platform/loader/fetch/Resource.cpp

Project Member

Comment 3 by bugdroid1@chromium.org, Dec 13 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9b0a3407099993d8acaa29e01a90dcfa5c1de1c2

commit 9b0a3407099993d8acaa29e01a90dcfa5c1de1c2
Author: Hiroshige Hayashizaki <hiroshige@chromium.org>
Date: Wed Dec 13 23:06:02 2017

Fix/add unit tests related to CachedMetadataHandler

This CL updates unit tests as a preparation for
CachedMetadataHandler changes in [1].

This CL
- Calls Resource::SetResponse() in ScriptStreamerTest.cpp and
  V8ScriptRunnerTest.cpp, because [1] will depend on
  SetResponse().
- Sets ResourceResponse's URL, because CachedMetadataHandler
  depends on response URLs.
- Uses http: URLs instead of data: URLs in ResourceTest, because
  CachedMetadataHandler is not set for data: URLs.

These are also natural, as
- SetResponse() is called and response URLs are set in non-test code.
- revalidation never occurs on data: URLs.

This CL also adds ResourceTest.RevalidationFailed, and
adds unit test assertions related to CachedMetadataHandler.

[1] https://chromium-review.googlesource.com/786410

Bug:  784875 
Change-Id: Ia58d3c6c9b2ad20c502d8ed9c69d505aa73be03b
Reviewed-on: https://chromium-review.googlesource.com/812569
Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Nate Chapin <japhet@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523926}
[modify] https://crrev.com/9b0a3407099993d8acaa29e01a90dcfa5c1de1c2/third_party/WebKit/Source/bindings/core/v8/ScriptStreamerTest.cpp
[modify] https://crrev.com/9b0a3407099993d8acaa29e01a90dcfa5c1de1c2/third_party/WebKit/Source/bindings/core/v8/V8ScriptRunnerTest.cpp
[modify] https://crrev.com/9b0a3407099993d8acaa29e01a90dcfa5c1de1c2/third_party/WebKit/Source/platform/loader/fetch/ResourceTest.cpp

Project Member

Comment 4 by bugdroid1@chromium.org, Dec 16 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cecbff796c9b4a57a49eb98912c718959e54218c

commit cecbff796c9b4a57a49eb98912c718959e54218c
Author: Hiroshige Hayashizaki <hiroshige@chromium.org>
Date: Sat Dec 16 00:57:44 2017

Create CachedMetadataHandler when a response is received

This CL creates CachedMetadataHandler when a response is received,
instead of creating when a Resource is constructed.

This is to
- Prepare for removing the reference from CachedMetadataHandler to
  Resource in [1]. The data needed for CachedMetadataHandler is
  available when a response is received, but not available when
  the Resource is created. Also, this CL makes the lifetime of
  CachedMetadataHandler match with the period where the corresponding
  ResourceResponse is set in Resource::response_.
- Enable CachedMetadataHandler even after revalidation.
  Previously, CachedMetadataHandler was cleared on failed revalidation
  and was never re-created after that.
  This CL creates a new CachedMetadataHandler everytime a
  ResourceResponse is received, and thus re-creates a
  CachedMetadataHandler on failed revalidation.

[1] https://chromium-review.googlesource.com/791494

Bug:  784875 
Change-Id: Ia32f5772cbd5970734f67098f235c2a1428b46ea
Reviewed-on: https://chromium-review.googlesource.com/786410
Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org>
Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Nate Chapin <japhet@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524540}
[modify] https://crrev.com/cecbff796c9b4a57a49eb98912c718959e54218c/third_party/WebKit/Source/platform/loader/fetch/Resource.cpp
[modify] https://crrev.com/cecbff796c9b4a57a49eb98912c718959e54218c/third_party/WebKit/Source/platform/loader/fetch/ResourceTest.cpp

Project Member

Comment 5 by bugdroid1@chromium.org, Jan 30 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f35da1e67a72557eece061711241cc10e1572e22

commit f35da1e67a72557eece061711241cc10e1572e22
Author: Hiroshige Hayashizaki <hiroshige@chromium.org>
Date: Tue Jan 30 05:58:32 2018

Decouple CachedMetadataHandler from Resource

This CL removes a reference from CachedMetadataHandlerImpl to
Resource, and instead captures necessary data from Resource
when CachedMetadataHandlerImpl is created, i.e. when a response
is received.

This is to make CachedMetadataHandler and Resource independent,
so that CachedMetadataHandler can be used even when the
corresponding Resource is garbage-collected or revalidated,
for Issue 783124.

This CL causes a behavior change in CachedMetadataHandler::Encoding():
Previously, it returned the final encoding that reflected the
encoding detection by the response body.
After this CL, it returns the encoding at the time of
ResponseReceived() that doesn't reflect the encoding detection
(by UTF BOMs in the case of scripts).
This shouldn't affect the correctness, because if the encoding at
the time of ResponseReceived() is the same, then the final encoding
should be also the same (provided the response content is the same).

This might cause different Encoding()s are returned for the same
final encoding and thus affect memory/performance, but this is
expected to be quite rare because UTF BOMs are quite rare.

Bug:  784875 , 783124
Change-Id: I0b72fafc54537fae93c90d1abf3bddf4e094add8
Reviewed-on: https://chromium-review.googlesource.com/791494
Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532784}
[modify] https://crrev.com/f35da1e67a72557eece061711241cc10e1572e22/third_party/WebKit/Source/platform/loader/fetch/Resource.cpp

Status: Fixed (was: Assigned)

Sign in to add a comment