Should not allow style sharing if grandparents matches different rule chain

Should not allow style sharing if grandparents matches different rule chain

This patch fixes a regression introduced in
https://chromiumcodereview.appspot.com/24106007

Before the regressing CL, the style sharing finder strictly checks style
object identity for every elements on the ancestor chain to allow sharing.
The regressing CL changed that so only the parent element is checked.

An optimization in Element::recalcOwnStyle allows previously shared style to
keep sharing if the computed style hasn't changed, even if the parent no
longer shares style. The mix of both causes problem with descendant elements
to misblieve they can share style when their grandparents now matches
different rule chain.

This patch disallows the optimization, so we have the invariant
(a->renderStyle() == b->renderStyle()) implies (a->parentElement()->renderStyle() == b->parentElement()->renderStyle())

BUG=296882
R=leviw@chromium.org

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

[trchen, 2013-10-19 06:28:46]

Hello reviewers,
This patch fixes a CSS style sharing regression that is very visible on tablet
gmail. PTAL. Thanks!

[trchen, 2013-10-19 06:30:34]

The CL description is for patchset 1.
I uploaded an alternative fix that adds check in SharedStyleFinder. Please let
me know which solution do you prefer. Thanks!

[esprehn, 2013-10-19 09:10:09]

I'm partial to patch set 1 since its simpler, although I suppose it does mean
we'll "undo" the sharing when we Force back down the tree in some cases.

If we do go with patch set 2 you need some null checking. :)

https://codereview.chromium.org/30453002/diff/120001/Source/core/css/resolver...
File Source/core/css/resolver/SharedStyleFinder.cpp (right):

https://codereview.chromium.org/30453002/diff/120001/Source/core/css/resolver...
Source/core/css/resolver/SharedStyleFinder.cpp:168: parentB =
parentB->parentElement();
I don't think this is safe. If parentA has a null parent and parentB does not
you'll crash on line 164.

[leviw_travelin_and_unemployed, 2013-10-21 18:02:09]

Your patch is certainly simpler, but I worry it's more expensive in the common
case. I'd be inclined to take your patch in the near-term while I work on
improving my solution https://codereview.chromium.org/29633003/

[trchen, 2013-10-21 18:48:16]

On 2013/10/21 18:02:09, Levi wrote:
> Your patch is certainly simpler, but I worry it's more expensive in the common
> case. I'd be inclined to take your patch in the near-term while I work on
> improving my solution https://codereview.chromium.org/29633003/

I thought your patch is almost finished? If the problem can be fixed before the
branch point, I think that is fine.

I'm also more inclined to use your solution. It eliminates the assumption about
style object identity (the old share style finder needed a hack in Element to
work? doesn't sound right). I think in the long term your solution will result
in cleaner and more maintainable code.

My only worry is that your patch needs to allocate more objects on the heap
during style resolving. In comparison, the patchset 2 needs to traverse the
whole ancestor chain in some cases, but 1. how often it actually needs to
traverse the whole ancestor chain and 2. in real world, how deep are the DOM
trees usually? I feel it is non-obvious which one is faster.

[esprehn, 2013-10-22 16:15:55]

I think we should land Patch 1 to fix the regression with a FIXME while we try
to figure out how to fix this. leviw's patch won't work, it creates a Vector the
size of the whole stylesheet and matches every rule in the entire sheet :(

The LRU is also in M31, so this regression is shipping. I think we should land
the simple fix for M32 while we try to figure out the path forward.

[trchen, 2013-10-22 21:33:50]

Agree. New CL uploaded.

[esprehn, 2013-10-22 21:43:51]

Blah, old chunk mismatch. Try uploading again.

[trchen, 2013-10-22 21:48:07]

Working this time. Stupid git-cl. :(

[esprehn, 2013-10-22 21:52:16]

lgtm

[commit-bot: I haz the power, 2013-10-22 22:00:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/trchen@chromium.org/30453002/280001

[commit-bot: I haz the power, 2013-10-22 23:16:39]

Change committed as 160267