Fix lfiespan of ScopedStyleResolver

Fix lifespan of ScopedStyleResolver

When a Shadow Tree is moved between different documents
(e.g. document <-> iframe), ScopedStyleResolver can remain
registered from its original document, which can result in
duplicate registration and possibly cause double-free etc.

This CL fixes it by clearing a shadow tree's
ScopedStyleResolver when the ShadowRoot is removed.

BUG=427249
TEST=pass the new layout test

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

[kochi, 2014-11-13 06:32:29]

kochi@chromium.org changed reviewers:
+ tasak@google.com

[kochi, 2014-11-13 06:35:34]

Hi Takashi,

Could you review?

[tasak, 2014-11-13 07:56:15]

lgtm

[esprehn, 2014-11-13 19:24:05]

esprehn@chromium.org changed reviewers:
+ esprehn@chromium.org

[esprehn, 2014-11-13 19:24:06]

Should we just implement ShadowRoot::didMoveToNewDocument?

https://codereview.chromium.org/721103002/diff/1/Source/core/dom/TreeScopeAdo...
File Source/core/dom/TreeScopeAdopter.cpp (right):

https://codereview.chromium.org/721103002/diff/1/Source/core/dom/TreeScopeAdo...
Source/core/dom/TreeScopeAdopter.cpp:144: // StyledScopeResolver may be null
because it is lazily created.
We probably need to clear the resolver and recompute it? Going between quirks
and non-quirks mode documents means we need to rebuild the stylesheet structures
anyway.

[kochi, 2014-11-14 05:38:39]

https://codereview.chromium.org/721103002/diff/1/Source/core/dom/TreeScopeAdo...
File Source/core/dom/TreeScopeAdopter.cpp (right):

https://codereview.chromium.org/721103002/diff/1/Source/core/dom/TreeScopeAdo...
Source/core/dom/TreeScopeAdopter.cpp:144: // StyledScopeResolver may be null
because it is lazily created.
On 2014/11/13 19:24:06, esprehn wrote:
> We probably need to clear the resolver and recompute it? Going between quirks
> and non-quirks mode documents means we need to rebuild the stylesheet
structures
> anyway.

Looks like this only happens on special case (e.g. <marquee>)
and now investigating the cause.

Probably what you said is correct, and this fix is just hiding the
real bug.

[tasak, 2014-11-14 12:37:39]

On 2014/11/13 19:24:06, esprehn wrote:
> Should we just implement ShadowRoot::didMoveToNewDocument?

I created a patch for this issue: https://codereview.chromium.org/730513002/

I think, we need to remove ScopedStyleResolver from StyleEngine when ShadowRoot
is removed from document.

[kochi, 2014-11-17 07:13:34]

Patchset #3 (id:40001) has been deleted

[kochi, 2014-11-17 07:48:36]

On 2014/11/14 12:37:39, tasak wrote:
> On 2014/11/13 19:24:06, esprehn wrote:
> > Should we just implement ShadowRoot::didMoveToNewDocument?
> 
> I created a patch for this issue: https://codereview.chromium.org/730513002/
> 
> I think, we need to remove ScopedStyleResolver from StyleEngine when
ShadowRoot
> is removed from document.

Thanks Takashi!

I've incorporated your fix and also added a layout test based on the minimized
crash case.
I'll see how it goes in trybots.

Once the other CL (https://codereview.chromium.org/722093002/) lands, I'll
update this.

[kochi, 2014-11-18 04:11:13]

Patchset #2 (id:20001) has been deleted

[kochi, 2014-11-18 04:11:23]

Patchset #2 (id:60001) has been deleted

[kochi, 2014-11-18 04:11:32]

Patchset #2 (id:80001) has been deleted

[kochi, 2014-11-18 04:11:41]

Patchset #2 (id:100001) has been deleted

[kochi, 2014-11-19 07:08:21]

Updated the description.

Takashi, please take another look.

[tasak, 2014-11-19 07:10:39]

lgtm

[kochi, 2014-11-19 08:10:52]

Thanks Takashi,

Elliott or Hayato, could you take OWNERS look?
Thanks,

[hayato, 2014-11-19 08:37:24]

Is there a reliable way to reproduce this issue? I guess not, however, it's
unclear to me why we need to reload many times and we need to have a <marque>
element to reproduce this.

I guess some comments or description might be helpful for the future readers to
understand the intention of the <marque> and reloading there.

[kochi, 2014-11-19 09:06:05]

On 2014/11/19 08:37:24, hayato wrote:
> Is there a reliable way to reproduce this issue? I guess not, however, it's
> unclear to me why we need to reload many times and we need to have a <marque>
> element to reproduce this.
> 
> I guess some comments or description might be helpful for the future readers
to
> understand the intention of the <marque> and reloading there.

This layout test can reliably reproduce the issue and crash.
See the trybot results of linux-blink-rel for Patchset 2.

<marquee> is special because it is implemented in Blink-in-JS and uses
custom elements and shadow DOM, and causes special race condition.
<marquee> is currently the only element which is implemented in Blink-in-JS.

Added a comment in the layout test.

[hayato, 2014-11-19 10:52:14]

hayato@chromium.org changed reviewers:
+ hayato@chromium.org

[hayato, 2014-11-19 10:52:15]

Thank you for adding the comment. LGTM

https://codereview.chromium.org/721103002/diff/180001/LayoutTests/fast/dom/St...
File
LayoutTests/fast/dom/StyleSheet/stylesheet-move-between-documents-crash.html
(right):

https://codereview.chromium.org/721103002/diff/180001/LayoutTests/fast/dom/St...
LayoutTests/fast/dom/StyleSheet/stylesheet-move-between-documents-crash.html:38:
marquee = document.createElement('marquee');
Looks 'var' is missing for marquee.

https://codereview.chromium.org/721103002/diff/180001/LayoutTests/fast/dom/St...
LayoutTests/fast/dom/StyleSheet/stylesheet-move-between-documents-crash.html:43:
iframe2.src='resources/stylesheet-move-iframe2.html';
spaces around '='.

https://codereview.chromium.org/721103002/diff/180001/Source/core/css/resolve...
File Source/core/css/resolver/StyleResolver.h (right):

https://codereview.chromium.org/721103002/diff/180001/Source/core/css/resolve...
Source/core/css/resolver/StyleResolver.h:214: void processScopedRules(const
RuleSet& authorRules, CSSStyleSheet*, unsigned sheetIndex, ContainerNode&
scope);
Looks unnecessary change.

[kochi, 2014-11-19 11:41:53]

Thanks for the review!

https://codereview.chromium.org/721103002/diff/180001/LayoutTests/fast/dom/St...
File
LayoutTests/fast/dom/StyleSheet/stylesheet-move-between-documents-crash.html
(right):

https://codereview.chromium.org/721103002/diff/180001/LayoutTests/fast/dom/St...
LayoutTests/fast/dom/StyleSheet/stylesheet-move-between-documents-crash.html:38:
marquee = document.createElement('marquee');
On 2014/11/19 10:52:15, hayato wrote:
> Looks 'var' is missing for marquee.

Done.

https://codereview.chromium.org/721103002/diff/180001/LayoutTests/fast/dom/St...
LayoutTests/fast/dom/StyleSheet/stylesheet-move-between-documents-crash.html:43:
iframe2.src='resources/stylesheet-move-iframe2.html';
On 2014/11/19 10:52:15, hayato wrote:
> spaces around '='.

Done.

https://codereview.chromium.org/721103002/diff/180001/Source/core/css/resolve...
File Source/core/css/resolver/StyleResolver.h (right):

https://codereview.chromium.org/721103002/diff/180001/Source/core/css/resolve...
Source/core/css/resolver/StyleResolver.h:214: void processScopedRules(const
RuleSet& authorRules, CSSStyleSheet*, unsigned sheetIndex, ContainerNode&
scope);
On 2014/11/19 10:52:15, hayato wrote:
> Looks unnecessary change.

Reverted.

[kochi, 2014-11-19 11:43:32]

Patchset #6 (id:200001) has been deleted

[kochi, 2014-11-19 11:46:00]

The CQ bit was checked by kochi@chromium.org

[commit-bot: I haz the power, 2014-11-19 11:46:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/721103002/220001

[commit-bot: I haz the power, 2014-11-19 12:54:56]

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

[commit-bot: I haz the power, 2014-11-19 12:54:57]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/3...)

[kochi, 2014-11-19 14:18:20]

The CQ bit was checked by kochi@chromium.org

[commit-bot: I haz the power, 2014-11-19 14:18:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/721103002/220001

[commit-bot: I haz the power, 2014-11-19 14:47:29]

Committed patchset #6 (id:220001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=185598