Move StyleSheetContents cache to StyleEngine.

Move StyleSheetContents cache to StyleEngine.

Blink should not share StyleSheetContents cache between different WebFrames.

If shares, WebFrameImpl::reload uses StyleSheetContents created by another WebFrameImpl.

So when changing allowImage setting (controlled by chrome) and reloading some page, the
reloaded frame might use StyleSheetContents cache.

Since StyleImages are cached in CSSValues and are not managed by FrameLoader, allowImage
change doesn't work.

BUG=346264
TEST=fast/css/background-image-reload.html, fast/css/two-different-iframe-not-share-style-image.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169414

[esprehn, 2014-02-27 09:25:11]

This isnt right, you're clearing the cache for all documents in the entire
process. Reload should also be tearing down the document which should
release the cached value?

If there's anything inside StyleSheetContents that is per document
something is busted. How does the <link> cache deal with this?

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[tasak, 2014-02-28 02:45:31]

Thank you for comments.

On 2014/02/27 09:25:11, esprehn wrote:
> This isnt right, you're clearing the cache for all documents in the entire
> process. Reload should also be tearing down the document which should
> release the cached value?

I would like to confirm my understanding:

(1) each tab has 1 renderer process, i.e. 1 Blink.
(2) reload is executed in per-tab manner, i.e. resetting 1 Blink.
(3) StyleEngine has static local hashmaps for caching StyleSheetContents. So the
hashmaps are per-process, i.e. per-tab cache.

So when Chrome wants to reload some tab page, we should reset all cache created
by browser cache?

I agree that it would be better to clear StyleSheetContents when allowImage
setting is changed.
However, I think, the setting is owned by Chrome / content_shell.
Blink can only do something when reload interface is invoked.

> If there's anything inside StyleSheetContents that is per document
> something is busted. How does the <link> cache deal with this?

Do you mean CSSStyleSheetResource?

As far as I investigated, the resource is managed by ResourceLoader.

Reload requests ResourceLoader to reload all remote contents.

However, unfortunately, url(data:png,image...) is not remote one.

After loading some css which contains such image, ResourceLoader does not work
because of StyleSheetContents cache.
StyleImage will be never recreated and no image will be shown (c.f.
crbug.com/346264).

[tasak, 2014-02-28 05:21:07]

On 2014/02/28 02:45:31, tasak wrote:

> (2) reload is executed in per-tab manner, i.e. resetting 1 Blink.

I mean, the "reload" is executed by "pressing F5 key" or "pressing browser's
reload button".

[tasak, 2014-03-04 03:04:28]

I chatted with shinyak@ and I found I missed the case where multiple tabs share
1 render process if a user opens a lot of tabs.

So I modified the clearCache to take 1 parameter (const LocalFrame*).

[tasak, 2014-03-05 09:09:25]

Would you review this CL?

The newest patch clears StyleContentsContents Cache only for a given frame.

[esprehn, 2014-03-05 09:58:08]

sure, I'll look today.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[tasak, 2014-03-05 11:32:41]

On 2014/03/05 09:58:08, esprehn wrote:
> sure, I'll look today.
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:blink-reviews+unsubscribe@chromium.org.

Thank you.

[tasak, 2014-03-05 13:06:58]

I'm sorry.

I found a mistake. I have to disable sharing StyleSheetContents between
different frames.
I will upload a new patch tomorrow (JST).

I' m now thinking of  changing textToSheet and sheetToText's value to have
LocalFrame*.
 (e.g. std_pair<LocalFrame*, StyleSheetContents*>, std_pair<LocalFrame*,
AtomicString>)

Best regards,
Takashi Sakamoto


2014-03-05 20:32 GMT+09:00 <tasak@google.com>:

> On 2014/03/05 09:58:08, esprehn wrote:
>
>> sure, I'll look today.
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:blink-reviews+unsubscribe@chromium.org.
>>
>
> Thank you.
>
>
> https://codereview.chromium.org/179873014/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[esprehn, 2014-03-06 06:52:39]

Instead lets just move the cache to Document (or StyleEngine), that also lets us
fix the media query bug where they disable the cache which is important for
polymer apps. LocalFrame is the wrong abstraction, sheets shouldn't need to know
anything about them.

[tasak, 2014-03-06 07:35:38]

Thank you for reviewing.

On 2014/03/06 06:52:39, esprehn wrote:
> Instead lets just move the cache to Document (or StyleEngine), that also lets
us
> fix the media query bug where they disable the cache which is important for
> polymer apps. LocalFrame is the wrong abstraction, sheets shouldn't need to
know
> anything about them.

I see.
I will try to move the cache.

So talking about the patch, I found we cannot assume that elements always have
frame(e.g. fast/dom/Document/cloneNode.html).
I agree that the patch is not good.

[tasak, 2014-03-06 12:33:19]

I have just uploaded the new patch: moving StyleSheetContents cache to
StyleEngine.

[dglazkov, 2014-03-06 18:43:59]

https://codereview.chromium.org/179873014/diff/70001/Source/core/css/CSSStyle...
File Source/core/css/CSSStyleSheet.h (right):

https://codereview.chromium.org/179873014/diff/70001/Source/core/css/CSSStyle...
Source/core/css/CSSStyleSheet.h:88: Document* ownerDocument(bool force = false)
const;
EEek. A wild ambiguously named bool appears!

[tasak, 2014-03-07 08:08:28]

https://codereview.chromium.org/179873014/diff/70001/Source/core/css/CSSStyle...
File Source/core/css/CSSStyleSheet.h (right):

https://codereview.chromium.org/179873014/diff/70001/Source/core/css/CSSStyle...
Source/core/css/CSSStyleSheet.h:88: Document* ownerDocument(bool force = false)
const;
On 2014/03/06 18:44:00, Dimitri Glazkov wrote:
> EEek. A wild ambiguously named bool appears!

I see.
I added a new method: ownerDocumentIgnoringDetached().

I think, the ownerDocument should not depend on m_ownerNode, because owner
document might be alive when clearOwnerNode() is invoked.

However, many codes uses ownerDocument() to obtain document and to check whether
stylesheet is detached or not.

[tasak, 2014-03-11 08:55:47]

Would you review this CL?

I moved StyleSheetContents cache to StyleEngine. So,

- Made CSSStyleSheet to have owner document (i.e. m_ownerDocument),
- Made StyleSheetContents::unregisterClient() to clear StyleSheetContents cache
by using the last CSSStyleSheet's owner document.

[dglazkov, 2014-03-11 16:30:18]

Overall, I think you have the right approach going. I have quibbles about the
tactics.

https://codereview.chromium.org/179873014/diff/120001/Source/core/css/CSSStyl...
File Source/core/css/CSSStyleSheet.cpp (right):

https://codereview.chromium.org/179873014/diff/120001/Source/core/css/CSSStyl...
Source/core/css/CSSStyleSheet.cpp:400: Document*
CSSStyleSheet::ownerDocumentIgnoringDetached() const
Now that CSSStyleSheet stores Document*, this seems a bit backwards.
ownerDocument() should always return m_document.

Then you need a bool thing that returns whether this stylesheet is attached,
right?

https://codereview.chromium.org/179873014/diff/120001/Source/core/css/CSSStyl...
Source/core/css/CSSStyleSheet.cpp:408: return root->ownerNode() ?
root->m_ownerDocument : 0;
Is there ever a case when root->m_ownerDocument != m_ownerDocument?

https://codereview.chromium.org/179873014/diff/120001/Source/core/css/CSSStyl...
File Source/core/css/CSSStyleSheet.h (right):

https://codereview.chromium.org/179873014/diff/120001/Source/core/css/CSSStyl...
Source/core/css/CSSStyleSheet.h:141: Document* m_ownerDocument;
Should we be clearing m_ownerDocument when the document is destroyed?

[tasak, 2014-03-12 13:02:32]

Thank you for reviewing.

I think, we should clear all m_ownerDocument in CSSStyleSheet when document is
being destroyed.
However, it would be a little difficult.

So I added m_activeClients to StyleSheetContents.

The m_activeClients has all active registered clients.
So we can use the active clients' ownerDocument to clear StyleSheetContents
cache.
If the m_activeClients becomes empty, StyleSheetContents cache for its
ownerDocument will be cleared.

https://codereview.chromium.org/179873014/diff/120001/Source/core/css/CSSStyl...
File Source/core/css/CSSStyleSheet.cpp (right):

https://codereview.chromium.org/179873014/diff/120001/Source/core/css/CSSStyl...
Source/core/css/CSSStyleSheet.cpp:400: Document*
CSSStyleSheet::ownerDocumentIgnoringDetached() const
On 2014/03/11 16:30:18, Dimitri Glazkov wrote:
> Now that CSSStyleSheet stores Document*, this seems a bit backwards.
> ownerDocument() should always return m_document.
> 
> Then you need a bool thing that returns whether this stylesheet is attached,
> right?

Right.
However, looking at code search result, I found I need to change a lot of
code...

So firstly I would like to fix this P1 issue quickly.
Next, I will modify ownerDocument() callers.

https://codereview.chromium.org/179873014/diff/120001/Source/core/css/CSSStyl...
Source/core/css/CSSStyleSheet.cpp:408: return root->ownerNode() ?
root->m_ownerDocument : 0;
On 2014/03/11 16:30:18, Dimitri Glazkov wrote:
> Is there ever a case when root->m_ownerDocument != m_ownerDocument?

I think, @import rules have rootStyleSheet(). Since @import rules use root
stylesheet's owner node, we can assume root->m_ownerDocument == m_ownerDocument.

However, I think, we have another concern: whether all registered clients have
the same document or not.

As far as I investigated,
- <LINK>:
  All registered clients don't have the same document. Loader caches
StyleSheetContents by using ParserContext (mainly, URL).

- <STYLE>:
  All registered clients have the same document, because StyleEngine caches
StyleSheetContents.

I added m_hasSingleOwnerDocument bit to StyleSheetContents to see whether a
newly registered sheet has the same document or not.

I also added ASSERT to StyleEngine::createSheet and checked whether cached
StyleSheetContents has the same document as the given style element or not.

https://codereview.chromium.org/179873014/diff/120001/Source/core/css/CSSStyl...
File Source/core/css/CSSStyleSheet.h (right):

https://codereview.chromium.org/179873014/diff/120001/Source/core/css/CSSStyl...
Source/core/css/CSSStyleSheet.h:141: Document* m_ownerDocument;
On 2014/03/11 16:30:18, Dimitri Glazkov wrote:
> Should we be clearing m_ownerDocument when the document is destroyed?

I see.
Yes, we should. I chatted with haraken@ and agreed that we need.

I think, we can probably create some crash test: CSSOM uses CSSStyleSheet but
<style> and document have been already destroyed.

...so I tried another approach.
I added m_activeClients to StyleSheetContents.
m_activeClients has all active (i.e. m_ownerNode != null) CSSSStyleSheet.
If m_activeClients becomes empty, clear StyleSheetContents cache.

[tasak, 2014-03-12 13:04:13]

https://codereview.chromium.org/179873014/diff/120001/Source/core/css/CSSStyl...
File Source/core/css/CSSStyleSheet.cpp (right):

https://codereview.chromium.org/179873014/diff/120001/Source/core/css/CSSStyl...
Source/core/css/CSSStyleSheet.cpp:400: Document*
CSSStyleSheet::ownerDocumentIgnoringDetached() const
On 2014/03/12 13:02:33, tasak wrote:

> So firstly I would like to fix this P1 issue quickly.
> Next, I will modify ownerDocument() callers.

I reverted ownerDocument()'s change.

[dglazkov, 2014-03-13 16:51:59]

lgtm with nits:

https://codereview.chromium.org/179873014/diff/160001/Source/core/css/CSSStyl...
File Source/core/css/CSSStyleSheet.cpp (right):

https://codereview.chromium.org/179873014/diff/160001/Source/core/css/CSSStyl...
Source/core/css/CSSStyleSheet.cpp:134: if (ownerNode)
This all can be moved to StyleSheetContents::registerClient, right?

https://codereview.chromium.org/179873014/diff/160001/Source/core/css/StyleSh...
File Source/core/css/StyleSheetContents.h (right):

https://codereview.chromium.org/179873014/diff/160001/Source/core/css/StyleSh...
Source/core/css/StyleSheetContents.h:198:
WillBeHeapHashSet<RawPtrWillBeWeakMember<CSSStyleSheet> > m_activeClients;
Do we _need_ both m_clients and m_activeClients?

[esprehn, 2014-03-13 21:11:57]

https://codereview.chromium.org/179873014/diff/160001/Source/core/css/StyleSh...
File Source/core/css/StyleSheetContents.cpp (right):

https://codereview.chromium.org/179873014/diff/160001/Source/core/css/StyleSh...
Source/core/css/StyleSheetContents.cpp:551: m_activeClients.add(sheet);
This seems like it overlaps with what Mads is doing.

https://codereview.chromium.org/179873014/diff/160001/Source/core/css/StyleSh...
Source/core/css/StyleSheetContents.cpp:569: m_hasSingleOwnerDocument = true;
How could something in the cache ever end up with clients from multiple
documents? It seems like you want a isCached bit and then deal with that.

https://codereview.chromium.org/179873014/diff/160001/Source/core/css/StyleSh...
File Source/core/css/StyleSheetContents.h (right):

https://codereview.chromium.org/179873014/diff/160001/Source/core/css/StyleSh...
Source/core/css/StyleSheetContents.h:145: void
unregisterActiveClient(CSSStyleSheet*);
Why do you need active clients?

https://codereview.chromium.org/179873014/diff/160001/Source/core/css/StyleSh...
Source/core/css/StyleSheetContents.h:154: void removeSheetFromCache(Document*);
This should be private?

https://codereview.chromium.org/179873014/diff/160001/Source/core/css/StyleSh...
Source/core/css/StyleSheetContents.h:198:
WillBeHeapHashSet<RawPtrWillBeWeakMember<CSSStyleSheet> > m_activeClients;
On 2014/03/13 16:52:00, dglazkov wrote:
> Do we _need_ both m_clients and m_activeClients?

I don't think you want both. Mads is about to another in a separate patch too,
and we certainly don't want 3 sets.

[tasak, 2014-03-14 07:16:19]

Thank you for reviewing.

https://codereview.chromium.org/179873014/diff/160001/Source/core/css/CSSStyl...
File Source/core/css/CSSStyleSheet.cpp (right):

https://codereview.chromium.org/179873014/diff/160001/Source/core/css/CSSStyl...
Source/core/css/CSSStyleSheet.cpp:134: if (ownerNode)
On 2014/03/13 16:52:00, dglazkov wrote:
> This all can be moved to StyleSheetContents::registerClient, right?

Done.

https://codereview.chromium.org/179873014/diff/160001/Source/core/css/StyleSh...
File Source/core/css/StyleSheetContents.cpp (right):

https://codereview.chromium.org/179873014/diff/160001/Source/core/css/StyleSh...
Source/core/css/StyleSheetContents.cpp:551: m_activeClients.add(sheet);
On 2014/03/13 21:11:58, esprehn wrote:
> This seems like it overlaps with what Mads is doing.

I think, his patch is for managing loaded sheets and loading sheets.
My patch is for managing active sheets and detached sheets.

However, now I changed m_clients semantics. We don't need active clients.

https://codereview.chromium.org/179873014/diff/160001/Source/core/css/StyleSh...
Source/core/css/StyleSheetContents.cpp:569: m_hasSingleOwnerDocument = true;
On 2014/03/13 21:11:58, esprehn wrote:
> How could something in the cache ever end up with clients from multiple
> documents? It seems like you want a isCached bit and then deal with that.

In my understanding, multiple documents cannot share the same StyleSheetContents
cache, because:
- StyleEngine has StyleSheetContents cache,
- StyleEngine is owned by document,
- each document has its own StyleEngine.

Talking about <link>, loader caches StyleSheetContents (by using URL). This is
not controlled by StyleEngine's cache.

This bit is for singleOwnerDocument(). Currently singleOwnerDocument doesn't
support multiple clients that have the same document.

https://codereview.chromium.org/179873014/diff/160001/Source/core/css/StyleSh...
File Source/core/css/StyleSheetContents.h (right):

https://codereview.chromium.org/179873014/diff/160001/Source/core/css/StyleSh...
Source/core/css/StyleSheetContents.h:145: void
unregisterActiveClient(CSSStyleSheet*);
On 2014/03/13 21:11:58, esprehn wrote:
> Why do you need active clients?

I changed m_clients to have only active CSSStyleSheets.

https://codereview.chromium.org/179873014/diff/160001/Source/core/css/StyleSh...
Source/core/css/StyleSheetContents.h:154: void removeSheetFromCache(Document*);
On 2014/03/13 21:11:58, esprehn wrote:
> This should be private?

When CSSStyleSheet will be mutated, we need to remove its StyleSheetContents
from StyleSheetContents cache.

https://codereview.chromium.org/179873014/diff/160001/Source/core/css/StyleSh...
Source/core/css/StyleSheetContents.h:198:
WillBeHeapHashSet<RawPtrWillBeWeakMember<CSSStyleSheet> > m_activeClients;
On 2014/03/13 21:11:58, esprehn wrote:
> On 2014/03/13 16:52:00, dglazkov wrote:
> > Do we _need_ both m_clients and m_activeClients?
> 
> I don't think you want both. Mads is about to another in a separate patch too,
> and we certainly don't want 3 sets.

I know his patch. I was afraid of changing m_clients' semantics (all registered
clients => all active clients).

[dglazkov, 2014-03-14 16:05:00]

lgtm++, esprehn what do you think?

https://codereview.chromium.org/179873014/diff/180001/Source/core/css/StyleSh...
File Source/core/css/StyleSheetContents.h (right):

https://codereview.chromium.org/179873014/diff/180001/Source/core/css/StyleSh...
Source/core/css/StyleSheetContents.h:143: bool hasOneClient() { return
clientSize() == 1; }
Is this function still necessary?

[esprehn, 2014-03-14 17:05:17]

lgtm, at some point we need to simplify this system way down and get rid of
singleOwnerDocument and singleOwnerNode and treat everything like it's shared,
but only at the per Document level.

[esprehn, 2014-03-14 23:23:22]

I've been pondering this more, and there's a much bigger issue here that's not
related to disabling images. Since the images are stored in the
StyleSheetContents, and the cache is based on the text of the CSS, if we cache
from two different pages in the same render process we'll share across them and
get the wrong image which is super bad.

ex.

google.com/a
<style>body{background: url(foo.png)}</style>

google.com/b
<style>body{background: url(foo.png)}</style>

The first should load google.com/a/foo.png and the second /b/foo.png, worse is
if the domains are different. For example if twitter.com and facebook.com have
the same stylesheet we'd load an image from the wrong one if they're in the same
render process (which is a bit rare on desktop, but very common on mobile).

This is potentially a privacy issue, but I suppose it's pretty rare that two
random origins have the exact same CSS.

Can you add a test for this? You should be able to do it with iframes and
different paths to the html files.

[tasak, 2014-03-17 11:59:50]

Thank you for reviewing.

I fixed patch conflict caused by loading/loadCompleted clients' patch.

> Can you add a test for this? You should be able to do it with iframes and
> different paths to the html files.

Sure. I added
LayoutTests/fast/css/two-different-iframe-not-share-style-image.html.

The test looks ok?

https://codereview.chromium.org/179873014/diff/180001/Source/core/css/StyleSh...
File Source/core/css/StyleSheetContents.h (right):

https://codereview.chromium.org/179873014/diff/180001/Source/core/css/StyleSh...
Source/core/css/StyleSheetContents.h:143: bool hasOneClient() { return
clientSize() == 1; }
On 2014/03/14 16:05:01, dglazkov wrote:
> Is this function still necessary?

I need to fix ScopedStyleResolver and StyleSheetContents::singleOwnerNode to
remove this method.
I will try in another CL.

[tasak, 2014-03-18 02:53:10]

The CQ bit was checked by tasak@google.com

[commit-bot: I haz the power, 2014-03-18 03:17:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tasak@google.com/179873014/220001

[commit-bot: I haz the power, 2014-03-18 04:02:47]

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

[commit-bot: I haz the power, 2014-03-18 04:02:48]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_rel

[esprehn, 2014-03-18 04:46:37]

The CQ bit was checked by esprehn@chromium.org

[commit-bot: I haz the power, 2014-03-18 04:46:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tasak@google.com/179873014/220001

[commit-bot: I haz the power, 2014-03-18 05:14:32]

Change committed as 169414