CSSStyleSheetResource should cache decoded text instead of raw bytes

third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp

 /*
     Copyright (C) 1998 Lars Knoll (knoll@mpi-hd.mpg.de)
     Copyright (C) 2001 Dirk Mueller (mueller@kde.org)
     Copyright (C) 2002 Waldo Bastian (bastian@kde.org)
     Copyright (C) 2006 Samuel Weinig (sam.weinig@gmail.com)
     Copyright (C) 2004, 2005, 2006 Apple Computer, Inc.
 
     This library is free software; you can redistribute it and/or
     modify it under the terms of the GNU Library General Public
     License as published by the Free Software Foundation; either
@@ skipping 25 matching lines @@
 #include "wtf/CurrentTime.h"
 
 namespace blink {
 
 CSSStyleSheetResource* CSSStyleSheetResource::fetch(FetchRequest& request,
                                                     ResourceFetcher* fetcher) {
   DCHECK_EQ(request.resourceRequest().frameType(),
             WebURLRequest::FrameTypeNone);
   request.mutableResourceRequest().setRequestContext(
       WebURLRequest::RequestContextStyle);
-  return toCSSStyleSheetResource(
+  CSSStyleSheetResource* resource = toCSSStyleSheetResource(
       fetcher->requestResource(request, CSSStyleSheetResourceFactory()));
+  if (resource && !request.integrityMetadata().isEmpty())
+    resource->setIntegrityMetadata(request.integrityMetadata());

[Charlie Harrison, 2016/10/05 15:24:48]

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.

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

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]

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:02:36]

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

+  return resource;
 }
 
 CSSStyleSheetResource* CSSStyleSheetResource::createForTest(
     const ResourceRequest& request,
     const String& charset) {
   return new CSSStyleSheetResource(request, ResourceLoaderOptions(), charset);
 }
 
 CSSStyleSheetResource::CSSStyleSheetResource(
     const ResourceRequest& resourceRequest,
@@ skipping 34 matching lines @@
     static_cast<StyleSheetResourceClient*>(c)->didAppendFirstData(this);
 
   // |c| might be removed in didAppendFirstData, so ensure it is still a client.
   if (hasClient(c) && !isLoading())
     static_cast<StyleSheetResourceClient*>(c)->setCSSStyleSheet(
         resourceRequest().url(), response().url(), encoding(), this);
 }
 
 const String CSSStyleSheetResource::sheetText(
     MIMETypeCheck mimeTypeCheck) const {
-  if (!data() || data()->isEmpty() || !canUseSheet(mimeTypeCheck))
+  if (!canUseSheet(mimeTypeCheck))
     return String();
 
-  if (!m_decodedSheetText.isNull())
+  // Use cached decoded sheet text when available
+  if (!m_decodedSheetText.isNull()) {
+    // We should have the decoded sheet text cached when the resource is fully loaded.
+    DCHECK_EQ(getStatus(), Resource::Cached);
+
     return m_decodedSheetText;
+  }
 
-  // Don't cache the decoded text, regenerating is cheap and it can use quite a
-  // bit of memory
+  if (!data() || data()->isEmpty())
+    return String();
+
   return decodedText();
 }
 
 void CSSStyleSheetResource::appendData(const char* data, size_t length) {
   Resource::appendData(data, length);
   if (m_didNotifyFirstData)
     return;
   ResourceClientWalker<StyleSheetResourceClient> w(clients());
   while (StyleSheetResourceClient* c = w.next())
     c->didAppendFirstData(this);
   m_didNotifyFirstData = true;
 }
 
 void CSSStyleSheetResource::checkNotify() {
-  // Decode the data to find out the encoding and keep the sheet text around
-  // during checkNotify()
-  if (data())
+  // Decode the data to find out the encoding and cache the decoded sheet text.
+  if (data()) {
     m_decodedSheetText = decodedText();
+  }
 
   ResourceClientWalker<StyleSheetResourceClient> w(clients());
   while (StyleSheetResourceClient* c = w.next()) {
     markClientFinished(c);
     c->setCSSStyleSheet(resourceRequest().url(), response().url(), encoding(),
                         this);
   }
-  // Clear the decoded text as it is unlikely to be needed immediately again and
-  // is cheap to regenerate.
-  m_decodedSheetText = String();
+
+  // Clear raw bytes as now we have the full decoded sheet text.
+  // We wait until all setCSSStyleSheet to run as SubresourceIntegrity checks require raw bytes.

[Charlie Harrison, 2016/10/05 15:24:48]

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.

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

nit: "until all" -> "for all"

[kouhei (in TOK), 2016/10/06 12:02:36]

Done.

[kouhei (in TOK), 2016/10/06 12:02:36]

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().

+  clearData();
 }
 
 void CSSStyleSheetResource::destroyDecodedDataIfPossible() {
   if (!m_parsedStyleSheetCache)
     return;
 
   setParsedStyleSheetCache(nullptr);
   setDecodedSize(0);
 }
 
+void CSSStyleSheetResource::destroyDecodedDataForFailedRevalidation() {
+  m_decodedSheetText = String();
+  destroyDecodedDataIfPossible();
+}
+
 bool CSSStyleSheetResource::canUseSheet(MIMETypeCheck mimeTypeCheck) const {
   if (errorOccurred())
     return false;
 
   // This check exactly matches Firefox. Note that we grab the Content-Type
   // header directly because we want to see what the value is BEFORE content
   // sniffing. Firefox does this by setting a "type hint" on the channel. This
   // implementation should be observationally equivalent.
   //
   // This code defaults to allowing the stylesheet for non-HTTP protocols so
@@ skipping 35 matching lines @@
     // This stylesheet resource did conflict with another resource and was not
     // added to the cache.
     setParsedStyleSheetCache(nullptr);
     return;
   }
   setParsedStyleSheetCache(sheet);
   setDecodedSize(m_parsedStyleSheetCache->estimatedSizeInBytes());
 }
 
 }  // namespace blink