Null check collection handling dirty scopes.

Null check collection handling dirty scopes.

When a shadow subtree containing a style element with a loading @import is
removed from the DOM, StyleEngine::removePendingSheet() will be called,
which in turn calls markTreeScopeDirty(). The collection has already been
removed from m_styleSheetCollectionMap in StyleEngine has already been for
the shadow root, which will lead to the collection being null when the
dirty tree scopes are being handled in
StyleEngine::updateActiveStyleSheetsInShadow().

Check that the node is in the Document before calling markTreeScopeDirty().

R=tasak@chromium.org
BUG=482087, 468267, 463031
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=194733

[rune, 2015-04-29 14:20:03]

rune@opera.com changed reviewers:
+ tasak@google.com
- tasak@chromium.org

[tasak, 2015-04-30 01:36:02]

FYI:
https://codereview.chromium.org/1105383002

I think, we should not mark treescope dirty when a given style element is not in
document.

[dominicc (has gone to gerrit), 2015-04-30 04:06:45]

On 2015/04/30 at 01:36:02, tasak wrote:
> FYI:
> https://codereview.chromium.org/1105383002
> 
> I think, we should not mark treescope dirty when a given style element is not
in document.

Can we pursue something that blends these two patches?

Can this test be included in https://codereview.chromium.org/1105383002 ?

Can some assertion move earlier/higher level than ASSERT(collection)?

[tasak, 2015-04-30 06:24:30]

On 2015/04/30 04:06:45, dominicc wrote:
> On 2015/04/30 at 01:36:02, tasak wrote:
> > FYI:
> > https://codereview.chromium.org/1105383002
> > 
> > I think, we should not mark treescope dirty when a given style element is
not
> in document.
> 
> Can we pursue something that blends these two patches?

I think, rune@ and I are looking at the same issue. So one of patches will fix
the issues. We don't need both.

> Can this test be included in https://codereview.chromium.org/1105383002 ?

I have already tested this test in https://codereview.chromium.org/1105383002
and saw no assert crashes.

> Can some assertion move earlier/higher level than ASSERT(collection)?

One idea: check whether we have any collection or not in markTreeScopeDirty.

[rune, 2015-04-30 07:47:57]

On 2015/04/30 06:24:30, tasak wrote:

> One idea: check whether we have any collection or not in markTreeScopeDirty.

Done in patch set 2.

I've modified the description accordingly.

[tasak, 2015-04-30 08:16:20]

https://codereview.chromium.org/1106313003/diff/20001/Source/core/dom/StyleEn...
File Source/core/dom/StyleEngine.cpp (right):

https://codereview.chromium.org/1106313003/diff/20001/Source/core/dom/StyleEn...
Source/core/dom/StyleEngine.cpp:246: markTreeScopeDirty(*treeScope);
I think, we should check the styleSheetCandidateNode is in document or not and
if not in document, skip this markTreeScopeDirty.
Because other code in StyleEngine.cpp, e.g. addStyleSheetCandidate, checks
in-document or not.

https://codereview.chromium.org/1106313003/diff/20001/Source/core/dom/StyleEn...
Source/core/dom/StyleEngine.cpp:582: if
(m_styleSheetCollectionMap.contains(&scope))
I think, it would be better to use ASSERT here, i.e.
ASSERT(m_styleSheetCollectionMap.contains(&scope));

[rune, 2015-04-30 08:35:45]

https://codereview.chromium.org/1106313003/diff/20001/Source/core/dom/StyleEn...
File Source/core/dom/StyleEngine.cpp (right):

https://codereview.chromium.org/1106313003/diff/20001/Source/core/dom/StyleEn...
Source/core/dom/StyleEngine.cpp:246: markTreeScopeDirty(*treeScope);
On 2015/04/30 08:16:20, tasak wrote:
> I think, we should check the styleSheetCandidateNode is in document or not and
> if not in document, skip this markTreeScopeDirty.
> Because other code in StyleEngine.cpp, e.g. addStyleSheetCandidate, checks
> in-document or not.

Done.

https://codereview.chromium.org/1106313003/diff/20001/Source/core/dom/StyleEn...
Source/core/dom/StyleEngine.cpp:582: if
(m_styleSheetCollectionMap.contains(&scope))
On 2015/04/30 08:16:20, tasak wrote:
> I think, it would be better to use ASSERT here, i.e.
> ASSERT(m_styleSheetCollectionMap.contains(&scope));

Done.

[tasak, 2015-04-30 09:11:10]

lgtm

[rune, 2015-04-30 09:14:46]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2015-04-30 09:15:25]

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

[commit-bot: I haz the power, 2015-04-30 11:58:12]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=194733