CSSStyleSheetResource should cache decoded text instead of raw bytes

CSSStyleSheetResource should cache decoded text instead of raw bytes

Before this CL, CSSStyleSheetResource kept undecoded text alive, and
decoded text whenever it was requested.

After this CL, the decoded text is kept alive as cache.
It discards undecoded text when the whole sheet text is loaded to avoid
unnecessary memory usage.

As a side-effect, CSSStyleSheetResource::sheetText is no longer const,
as it may cache m_decodedSheetText.

BUG=642722, 653502
Committed: https://crrev.com/afa48c3ed31f7229c306ce7cda62111be593f719
Cr-Commit-Position: refs/heads/master@{#424941}

[kouhei (in TOK), 2016-08-31 07:09:35]

Description was changed from

==========
CSSStyleSheetResource should cache decoded text instead of raw bytes

Before this CL, CSSStyleSheetResource kept undecoded text alive, and
decoded text whenever it was requested.

After this CL, the decoded text is kept alive as cache.
It discards undecoded text when the whole sheet text is loaded to avoid
unnecessary memory usage.

BUG=None
==========

to

==========
CSSStyleSheetResource should cache decoded text instead of raw bytes

Before this CL, CSSStyleSheetResource kept undecoded text alive, and
decoded text whenever it was requested.

After this CL, the decoded text is kept alive as cache.
It discards undecoded text when the whole sheet text is loaded to avoid
unnecessary memory usage.

As a side-effect, CSSStyleSheetResource::sheetText is no longer const,
as it may cache m_decodedSheetText.

BUG=None
==========

[kouhei (in TOK), 2016-08-31 07:10:48]

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

[kouhei (in TOK), 2016-08-31 07:10:48]

Would you take a look?

[kouhei (in TOK), 2016-08-31 07:10:54]

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

[commit-bot: I haz the power, 2016-08-31 07:11:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2016-08-31 07:30:25]

https://codereview.chromium.org/2290983003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp (right):

https://codereview.chromium.org/2290983003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp:111: return
m_decodedSheetText;
IIUC, the Lines 106--111 are executed only in sheetText() called in
checkNotify().
How about splitting out this block to a new method and making checkNotify() to
call the new method, and keeping sheetText() const?
This will clarify when |m_decodedSheetText| is set, and also make this CL much
smaller.

https://codereview.chromium.org/2290983003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp:128: sheetText();
I think we call sheetText() here to ensure |m_data| is always null after this
point.
Please add a comment (perhaps somewhere in CssStyleSheetResource.h) about the
lifetime of |m_data| and |m_decodedSheetText|.

https://codereview.chromium.org/2290983003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.h (right):

https://codereview.chromium.org/2290983003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.h:79: void
destroyDecodedDataForFailedRevalidation() override {
destroyDecodedDataIfPossible(); }
We must clear |m_decodedSheetText| in destroyDecodedDataForFailedRevalidation().
Otherwise, the old stale |m_decodedSheetText| is used after revalidation (after
we receive response headers for revalidating request and before revalidating
loading is completed).
If bots are green, then it means we lack test cases...I'll create tests.

[commit-bot: I haz the power, 2016-08-31 08:54:17]

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

[commit-bot: I haz the power, 2016-08-31 08:54:18]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[kouhei (in TOK), 2016-08-31 10:27:43]

https://codereview.chromium.org/2290983003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp (right):

https://codereview.chromium.org/2290983003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp:111: return
m_decodedSheetText;
On 2016/08/31 07:30:25, hiroshige wrote:
> IIUC, the Lines 106--111 are executed only in sheetText() called in
> checkNotify().
> How about splitting out this block to a new method and making checkNotify() to
> call the new method, and keeping sheetText() const?
> This will clarify when |m_decodedSheetText| is set, and also make this CL much
> smaller.

Done.

https://codereview.chromium.org/2290983003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp:128: sheetText();
On 2016/08/31 07:30:25, hiroshige wrote:
> I think we call sheetText() here to ensure |m_data| is always null after this
> point.
> Please add a comment (perhaps somewhere in CssStyleSheetResource.h) about the
> lifetime of |m_data| and |m_decodedSheetText|.

Done.

https://codereview.chromium.org/2290983003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.h (right):

https://codereview.chromium.org/2290983003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.h:79: void
destroyDecodedDataForFailedRevalidation() override {
destroyDecodedDataIfPossible(); }
On 2016/08/31 07:30:25, hiroshige wrote:
> We must clear |m_decodedSheetText| in
destroyDecodedDataForFailedRevalidation().
> Otherwise, the old stale |m_decodedSheetText| is used after revalidation
(after
> we receive response headers for revalidating request and before revalidating
> loading is completed).
> If bots are green, then it means we lack test cases...I'll create tests.

Done. Tests seems to be failing as expected.

[kouhei (in TOK), 2016-08-31 10:27:52]

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

[commit-bot: I haz the power, 2016-08-31 10:28:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2016-08-31 10:34:53]

https://codereview.chromium.org/2290983003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp (right):

https://codereview.chromium.org/2290983003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp:138:
m_decodedSheetText = String();
Probably clearing |m_decodedSheetText| in destroyDecodedDataIfPossible() will
cause an error when prune() is called: after |m_decodedSheetText| is cleared,
the CSSStyleSheetResource is no longer reusable. Waiting for bot results.

[kouhei (in TOK), 2016-08-31 12:59:58]

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

[commit-bot: I haz the power, 2016-08-31 13:00:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2016-08-31 13:01:18]

csharrison@chromium.org changed reviewers:
+ csharrison@chromium.org
- csharrison@google.com

[Charlie Harrison, 2016-08-31 13:01:19]

Thanks for the patch! Feel free to add issue 642722 as BUG=, which I created to
track the lazy parsing work.

https://codereview.chromium.org/2290983003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp (right):

https://codereview.chromium.org/2290983003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp:101:
DCHECK(getStatus() != Resource::Cached);
nit: DCHECK_NE

https://codereview.chromium.org/2290983003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp:125: // Decode
the data to find out the encoding and cache the decoded sheet text.
Also comment that clearData() only clears the raw bytes.

https://codereview.chromium.org/2290983003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp:146: void
CSSStyleSheetResource::destroyDecodedDataIfPossible()
duplicate method?

[commit-bot: I haz the power, 2016-08-31 13:09:31]

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

[commit-bot: I haz the power, 2016-08-31 13:09:32]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[kouhei (in TOK), 2016-08-31 13:41:48]

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

[commit-bot: I haz the power, 2016-08-31 13:42:05]

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, 2016-08-31 14:54:11]

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

[commit-bot: I haz the power, 2016-08-31 14:54:13]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hiroshige, 2016-09-01 03:57:58]

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

[commit-bot: I haz the power, 2016-09-01 03:58:12]

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, 2016-09-01 05:10:57]

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

[commit-bot: I haz the power, 2016-09-01 05:10:58]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Charlie Harrison, 2016-09-01 16:52:05]

I wonder if we could decode the bytes as they stream in? I am thinking of
implementing a streaming tokenizer...

[kouhei (in TOK), 2016-09-02 03:36:44]

Description was changed from

==========
CSSStyleSheetResource should cache decoded text instead of raw bytes

Before this CL, CSSStyleSheetResource kept undecoded text alive, and
decoded text whenever it was requested.

After this CL, the decoded text is kept alive as cache.
It discards undecoded text when the whole sheet text is loaded to avoid
unnecessary memory usage.

As a side-effect, CSSStyleSheetResource::sheetText is no longer const,
as it may cache m_decodedSheetText.

BUG=None
==========

to

==========
CSSStyleSheetResource should cache decoded text instead of raw bytes

Before this CL, CSSStyleSheetResource kept undecoded text alive, and
decoded text whenever it was requested.

After this CL, the decoded text is kept alive as cache.
It discards undecoded text when the whole sheet text is loaded to avoid
unnecessary memory usage.

As a side-effect, CSSStyleSheetResource::sheetText is no longer const,
as it may cache m_decodedSheetText.

BUG=642722
==========

[Charlie Harrison, 2016-09-02 03:40:49]

https://codereview.chromium.org/2290983003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp (right):

https://codereview.chromium.org/2290983003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp:101:
DCHECK(getStatus() != Resource::Cached);
I think this DCHECK isn't quite right. We set the resource status to Cached
right before checkNotify.

[kouhei (in TOK), 2016-09-02 03:43:42]

https://codereview.chromium.org/2290983003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp (right):

https://codereview.chromium.org/2290983003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp:101:
DCHECK(getStatus() != Resource::Cached);
On 2016/08/31 13:01:18, Charlie Harrison wrote:
> nit: DCHECK_NE

Done.

https://codereview.chromium.org/2290983003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp:125: // Decode
the data to find out the encoding and cache the decoded sheet text.
On 2016/08/31 13:01:18, Charlie Harrison wrote:
> Also comment that clearData() only clears the raw bytes.

Done.

https://codereview.chromium.org/2290983003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp:146: void
CSSStyleSheetResource::destroyDecodedDataIfPossible()
On 2016/08/31 13:01:18, Charlie Harrison wrote:
> duplicate method?

Done.

https://codereview.chromium.org/2290983003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp:146: void
CSSStyleSheetResource::destroyDecodedDataIfPossible()
On 2016/08/31 13:01:18, Charlie Harrison wrote:
> duplicate method?

Done.

https://codereview.chromium.org/2290983003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp (right):

https://codereview.chromium.org/2290983003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp:101:
DCHECK(getStatus() != Resource::Cached);
On 2016/09/02 03:40:49, Charlie Harrison wrote:
> I think this DCHECK isn't quite right. We set the resource status to Cached
> right before checkNotify.

Yes, that was causing the test failures. Swapped in PS5

[kouhei (in TOK), 2016-09-02 03:43:51]

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

[commit-bot: I haz the power, 2016-09-02 03:43:58]

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, 2016-09-02 05:19:56]

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

[commit-bot: I haz the power, 2016-09-02 05:19:57]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Charlie Harrison, 2016-09-02 12:17:15]

This is breaking the @import css scanner, which calls decodedText() in
appendData() callbacks.

Two solutions:
1. Turn off @import scanner for now, as I think the lazy parsing is a much more
promising win.
2. Decode the bytes bit by bit in appendData (this will also remove a string
copy from the @import scanner).

[kouhei (in TOK), 2016-09-02 13:55:56]

On 2016/09/02 12:17:15, Charlie Harrison wrote:
> This is breaking the @import css scanner, which calls decodedText() in
> appendData() callbacks.
> 
> Two solutions:
> 1. Turn off @import scanner for now, as I think the lazy parsing is a much
more
> promising win.
> 2. Decode the bytes bit by bit in appendData (this will also remove a string
> copy from the @import scanner).

Hmm. I replaced that with sheetText() and CSSPreloadScanner now works.

However, I got bitten by SubresourceIntegrity checks, where it requires raw
bytes to compute the content hash. Let me see if there is any workaround for
that.

[Charlie Harrison, 2016-09-02 13:59:27]

On 2016/09/02 13:55:56, kouhei wrote:
> On 2016/09/02 12:17:15, Charlie Harrison wrote:
> > This is breaking the @import css scanner, which calls decodedText() in
> > appendData() callbacks.
> > 
> > Two solutions:
> > 1. Turn off @import scanner for now, as I think the lazy parsing is a much
> more
> > promising win.
> > 2. Decode the bytes bit by bit in appendData (this will also remove a string
> > copy from the @import scanner).
> 
> Hmm. I replaced that with sheetText() and CSSPreloadScanner now works.
Ah right, there is a fallback to regenerating the decoded text. SGTM.

> 
> However, I got bitten by SubresourceIntegrity checks, where it requires raw
> bytes to compute the content hash. Let me see if there is any workaround for
> that.

[kouhei (in TOK), 2016-09-14 20:26:40]

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

[commit-bot: I haz the power, 2016-09-14 20:27:21]

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, 2016-09-14 21:37:10]

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

[commit-bot: I haz the power, 2016-09-14 21:37:11]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Charlie Harrison, 2016-09-15 14:06:13]

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

[commit-bot: I haz the power, 2016-09-15 14:06:30]

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, 2016-09-15 15:25:11]

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

[commit-bot: I haz the power, 2016-09-15 15:25:14]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2016-09-22 18:03:51]

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

[commit-bot: I haz the power, 2016-09-22 18:04:22]

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, 2016-09-22 18:07:31]

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

[commit-bot: I haz the power, 2016-09-22 18:07:32]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Charlie Harrison, 2016-10-03 18:24:59]

What is the current status here?

[kouhei (in TOK), 2016-10-05 08:11:37]

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

[commit-bot: I haz the power, 2016-10-05 08:11:53]

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, 2016-10-05 09:43:01]

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

[commit-bot: I haz the power, 2016-10-05 09:43:02]

Dry run: This issue passed the CQ dry run.

[kouhei (in TOK), 2016-10-05 13:44:50]

kouhei@chromium.org changed reviewers:
+ jww@chromium.org

[kouhei (in TOK), 2016-10-05 13:44:51]

I think the CL is now ready for first pass. Would you take a look?

[kouhei (in TOK), 2016-10-05 13:52:28]

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

[commit-bot: I haz the power, 2016-10-05 13:52:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2016-10-05 15:24:49]

Looks good, but can you update the description enumerating the changes to SRI?
Namely that we now cache integrity metadata and disposition at the Resource
level.

https://codereview.chromium.org/2290983003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp (right):

https://codereview.chromium.org/2290983003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp:49:
resource->setIntegrityMetadata(request.integrityMetadata());
It feels like setting the integrity metadata should now be resource type
agnostic (and possible live in ResourceFetcher::requestResource. I don't feel
too strongly about this though.

https://codereview.chromium.org/2290983003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp:146: // We wait
until all setCSSStyleSheet to run as SubresourceIntegrity checks require raw
bytes.
Is it possible for setCSSStyleSheet to be run on a sheet after this clear() is
called?

Most cases I can tell will end up loading from memory cache which is safe (we
check that the metadata is equal), so it might be worth a comment.

https://codereview.chromium.org/2290983003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLLinkElement.cpp (right):

https://codereview.chromium.org/2290983003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLLinkElement.cpp:434: if (disposition ==
ResourceIntegrityDisposition::NotChecked)
nit: A conditional branch just for a DCHECK isn't great style. You can just
combine the conditions within the DCHECK:

DCHECK(disposition != ResourceIntegrityDisposition::NotChecked || 
       cachedStyleSheet->sheetText().isNull());

(too bad DCHECK_IMPLIES got removed from the codebase)

https://codereview.chromium.org/2290983003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLLinkElement.cpp:447:
const_cast<CSSStyleSheetResource*>(cachedStyleSheet)
The const_cast is not ideal, but I can't think of a way around it without
significant refactors.

Ideas:
 - Change setCSSStyleSheet to return the disposition, and cache it in the
resource from the caller.
 - Call it out for removal when crbug.com/500701 is addressed with a comment.

[jww, 2016-10-05 15:32:49]

Please add a similar test to
third_party/WebKit/LayoutTests/http/tests/security/subresourceIntegrity/subresource-integrity-load-regular-script-after-failed-integrity.html
but which covers the stylesheet variant. This is a test for precisely the case
that you're changing.

Once that's in, the code lgtm. Thanks!

https://codereview.chromium.org/2290983003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp (right):

https://codereview.chromium.org/2290983003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp:146: // We wait
until all setCSSStyleSheet to run as SubresourceIntegrity checks require raw
bytes.
nit: "until all" -> "for all"

[commit-bot: I haz the power, 2016-10-05 17:26:00]

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

[commit-bot: I haz the power, 2016-10-05 17:26:01]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[kouhei (in TOK), 2016-10-05 23:38:22]

Thanks for review comments!

> Looks good, but can you update the description enumerating the changes to SRI?
> Namely that we now cache integrity metadata and disposition at the Resource
> level.
I'm planning to split this CL for changes that can be split. (I like small CLs)

[jww, 2016-10-06 01:55:04]

https://codereview.chromium.org/2290983003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp (right):

https://codereview.chromium.org/2290983003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp:49:
resource->setIntegrityMetadata(request.integrityMetadata());
On 2016/10/05 15:24:48, Charlie Harrison wrote:
> It feels like setting the integrity metadata should now be resource type
> agnostic (and possible live in ResourceFetcher::requestResource. I don't feel
> too strongly about this though.

I wrote this code a while back, so my memory isn't perfect, but I believe the
complication is with scripts and when the data is actually available to
calculate the integrity is not when you expect, so it has to be done per
resource type.

[Charlie Harrison, 2016-10-06 03:59:08]

https://codereview.chromium.org/2290983003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp (right):

https://codereview.chromium.org/2290983003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp:49:
resource->setIntegrityMetadata(request.integrityMetadata());
On 2016/10/06 01:55:04, jww wrote:
> On 2016/10/05 15:24:48, Charlie Harrison wrote:
> > It feels like setting the integrity metadata should now be resource type
> > agnostic (and possible live in ResourceFetcher::requestResource. I don't
feel
> > too strongly about this though.
> 
> I wrote this code a while back, so my memory isn't perfect, but I believe the
> complication is with scripts and when the data is actually available to
> calculate the integrity is not when you expect, so it has to be done per
> resource type.

I think you're right about calculating the integrity disposition, but I think
caching the integrity metadata is okay here. This logic is copied over from
ScriptResource::fetch.

That being said I'm still not convinced my suggestion is worthwhile :P

[kouhei (in TOK), 2016-10-06 12:00:41]

Description was changed from

==========
CSSStyleSheetResource should cache decoded text instead of raw bytes

Before this CL, CSSStyleSheetResource kept undecoded text alive, and
decoded text whenever it was requested.

After this CL, the decoded text is kept alive as cache.
It discards undecoded text when the whole sheet text is loaded to avoid
unnecessary memory usage.

As a side-effect, CSSStyleSheetResource::sheetText is no longer const,
as it may cache m_decodedSheetText.

BUG=642722
==========

to

==========
CSSStyleSheetResource should cache decoded text instead of raw bytes

Before this CL, CSSStyleSheetResource kept undecoded text alive, and
decoded text whenever it was requested.

After this CL, the decoded text is kept alive as cache.
It discards undecoded text when the whole sheet text is loaded to avoid
unnecessary memory usage.

As a side-effect, CSSStyleSheetResource::sheetText is no longer const,
as it may cache m_decodedSheetText.

BUG=642722, 653502
==========

[kouhei (in TOK), 2016-10-06 12:02:37]

I'll work on test tomorrow JST

https://codereview.chromium.org/2290983003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp (right):

https://codereview.chromium.org/2290983003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp:49:
resource->setIntegrityMetadata(request.integrityMetadata());
On 2016/10/06 03:59:08, Charlie Harrison wrote:
> On 2016/10/06 01:55:04, jww wrote:
> > On 2016/10/05 15:24:48, Charlie Harrison wrote:
> > > It feels like setting the integrity metadata should now be resource type
> > > agnostic (and possible live in ResourceFetcher::requestResource. I don't
> feel
> > > too strongly about this though.
> > 
> > I wrote this code a while back, so my memory isn't perfect, but I believe
the
> > complication is with scripts and when the data is actually available to
> > calculate the integrity is not when you expect, so it has to be done per
> > resource type.
> 
> I think you're right about calculating the integrity disposition, but I think
> caching the integrity metadata is okay here. This logic is copied over from
> ScriptResource::fetch.
> 
> That being said I'm still not convinced my suggestion is worthwhile :P

Let me try this in separate CL. Added a TODO comment here.

https://codereview.chromium.org/2290983003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp:146: // We wait
until all setCSSStyleSheet to run as SubresourceIntegrity checks require raw
bytes.
On 2016/10/05 15:32:49, jww wrote:
> nit: "until all" -> "for all"

Done.

https://codereview.chromium.org/2290983003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp:146: // We wait
until all setCSSStyleSheet to run as SubresourceIntegrity checks require raw
bytes.
On 2016/10/05 15:24:48, Charlie Harrison wrote:
> Is it possible for setCSSStyleSheet to be run on a sheet after this clear() is
> called?
Yes. From CSSStyleSheetResource::didAddClient().
However, we are safe as the only consumer of the data() is
LinkStyle::setCSSStyleSheet, which is guaranteed to be called from
CSSStyleSheetResource::checkNotify().

https://codereview.chromium.org/2290983003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLLinkElement.cpp (right):

https://codereview.chromium.org/2290983003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLLinkElement.cpp:434: if (disposition ==
ResourceIntegrityDisposition::NotChecked)
On 2016/10/05 15:24:49, Charlie Harrison wrote:
> nit: A conditional branch just for a DCHECK isn't great style. You can just
> combine the conditions within the DCHECK:
> 
> DCHECK(disposition != ResourceIntegrityDisposition::NotChecked || 
>        cachedStyleSheet->sheetText().isNull());
> 
> (too bad DCHECK_IMPLIES got removed from the codebase)

Done.

[kouhei (in TOK), 2016-10-06 12:03:15]

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

[commit-bot: I haz the power, 2016-10-06 12:03:32]

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, 2016-10-06 12:18:50]

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

[commit-bot: I haz the power, 2016-10-06 12:18:51]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[kouhei (in TOK), 2016-10-06 13:46:26]

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

[commit-bot: I haz the power, 2016-10-06 13:46:48]

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, 2016-10-06 15:37:44]

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

[commit-bot: I haz the power, 2016-10-06 15:37:45]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[kouhei (in TOK), 2016-10-07 01:47:06]

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

[commit-bot: I haz the power, 2016-10-07 01:47:32]

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, 2016-10-07 01:56:37]

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

[commit-bot: I haz the power, 2016-10-07 01:56:38]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[kouhei (in TOK), 2016-10-07 04:08:30]

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

[commit-bot: I haz the power, 2016-10-07 04:08:40]

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, 2016-10-07 04:13:31]

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

[commit-bot: I haz the power, 2016-10-07 04:13:33]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[kouhei (in TOK), 2016-10-07 05:19:05]

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

[commit-bot: I haz the power, 2016-10-07 05:19:19]

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, 2016-10-07 05:24:16]

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

[commit-bot: I haz the power, 2016-10-07 05:24:17]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[kouhei (in TOK), 2016-10-11 00:56:28]

Added tests. PTAL.

[Charlie Harrison, 2016-10-11 03:32:34]

non-owner LGTM

[Yoav Weiss, 2016-10-11 08:28:15]

On 2016/10/11 03:32:34, Charlie Harrison wrote:
> non-owner LGTM

RSLGTM

[kouhei (in TOK), 2016-10-11 22:47:20]

The CQ bit was checked by kouhei@chromium.org

[kouhei (in TOK), 2016-10-11 22:47:21]

The patchset sent to the CQ was uploaded after l-g-t-m from jww@chromium.org
Link to the patchset: https://codereview.chromium.org/2290983003/#ps280001
(title: "add test")

[commit-bot: I haz the power, 2016-10-11 22:48:04]

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, 2016-10-11 23:00:28]

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

[commit-bot: I haz the power, 2016-10-11 23:00:31]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[kouhei (in TOK), 2016-10-13 01:54:42]

The CQ bit was checked by kouhei@chromium.org

[kouhei (in TOK), 2016-10-13 01:54:42]

The patchset sent to the CQ was uploaded after l-g-t-m from jww@chromium.org,
csharrison@chromium.org
Link to the patchset: https://codereview.chromium.org/2290983003/#ps300001
(title: "rebased")

[commit-bot: I haz the power, 2016-10-13 01:54:57]

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, 2016-10-13 03:05:04]

Description was changed from

==========
CSSStyleSheetResource should cache decoded text instead of raw bytes

Before this CL, CSSStyleSheetResource kept undecoded text alive, and
decoded text whenever it was requested.

After this CL, the decoded text is kept alive as cache.
It discards undecoded text when the whole sheet text is loaded to avoid
unnecessary memory usage.

As a side-effect, CSSStyleSheetResource::sheetText is no longer const,
as it may cache m_decodedSheetText.

BUG=642722, 653502
==========

to

==========
CSSStyleSheetResource should cache decoded text instead of raw bytes

Before this CL, CSSStyleSheetResource kept undecoded text alive, and
decoded text whenever it was requested.

After this CL, the decoded text is kept alive as cache.
It discards undecoded text when the whole sheet text is loaded to avoid
unnecessary memory usage.

As a side-effect, CSSStyleSheetResource::sheetText is no longer const,
as it may cache m_decodedSheetText.

BUG=642722, 653502
==========

[commit-bot: I haz the power, 2016-10-13 03:05:06]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2016-10-13 03:08:39]

Description was changed from

==========
CSSStyleSheetResource should cache decoded text instead of raw bytes

Before this CL, CSSStyleSheetResource kept undecoded text alive, and
decoded text whenever it was requested.

After this CL, the decoded text is kept alive as cache.
It discards undecoded text when the whole sheet text is loaded to avoid
unnecessary memory usage.

As a side-effect, CSSStyleSheetResource::sheetText is no longer const,
as it may cache m_decodedSheetText.

BUG=642722, 653502
==========

to

==========
CSSStyleSheetResource should cache decoded text instead of raw bytes

Before this CL, CSSStyleSheetResource kept undecoded text alive, and
decoded text whenever it was requested.

After this CL, the decoded text is kept alive as cache.
It discards undecoded text when the whole sheet text is loaded to avoid
unnecessary memory usage.

As a side-effect, CSSStyleSheetResource::sheetText is no longer const,
as it may cache m_decodedSheetText.

BUG=642722, 653502

Committed: https://crrev.com/afa48c3ed31f7229c306ce7cda62111be593f719
Cr-Commit-Position: refs/heads/master@{#424941}
==========

[commit-bot: I haz the power, 2016-10-13 03:08:41]

Patchset 16 (id:??) landed as
https://crrev.com/afa48c3ed31f7229c306ce7cda62111be593f719
Cr-Commit-Position: refs/heads/master@{#424941}