CSS: Additional asserts for style invalidation

CSS: Additional asserts for style invalidation

RuleFeatureSet::add now verifies that we aren't adding to ourselves.

Our hash sets now verify during insertion that our keys are
not empty strings or PseudoUnknown (0).

BUG=412572, 640486
Committed: https://crrev.com/a042e2557c51f8f9f71ea2e00e4ac1016426b242
Cr-Commit-Position: refs/heads/master@{#416220}

[esprehn, 2016-09-01 05:30:21]

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

[esprehn, 2016-09-01 05:30:22]

This seems a bit like whack-a-mole, why mess with the HashSet?

https://codereview.chromium.org/2303443003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/RuleFeature.h (right):

https://codereview.chromium.org/2303443003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/RuleFeature.h:126: using
PseudoTypeInvalidationSetMap = HashMap<int, RefPtr<InvalidationSet>>; // key
values are CSSSelector::PseudoType
Does this need changing in this patch?

https://codereview.chromium.org/2303443003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/invalidation/InvalidationSet.h (right):

https://codereview.chromium.org/2303443003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/invalidation/InvalidationSet.h:83:
USING_FAST_MALLOC(blink::InvalidationSet);
Why change this in this patch? The other macro seems about the same, I'm also
not sure why we annotate InvalidationSet and CSSSelector with
USING_FAST_MALLOC_WITH_TYPE_NAME but nothing else.

[Eric Willigers, 2016-09-01 14:53:15]

Description was changed from

==========
CSS: Additional asserts for style invalidation

RuleFeatureSet::add now verifies that we aren't adding to ourselves.

Our hash sets now verify during insertion that our keys are not empty
strings or PseudoUnknown (0).

We trial not using USING_FAST_MALLOC_WITH_TYPE_NAME.

BUG=412572,640486
==========

to

==========
CSS: Additional asserts for style invalidation

RuleFeatureSet::add now verifies that we aren't adding to ourselves.

Our hash sets now verify during insertion that our keys are
not empty strings or PseudoUnknown (0).

BUG=412572,640486
==========

[Eric Willigers, 2016-09-01 14:56:31]

https://codereview.chromium.org/2303443003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/RuleFeature.h (right):

https://codereview.chromium.org/2303443003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/RuleFeature.h:126: using
PseudoTypeInvalidationSetMap = HashMap<int, RefPtr<InvalidationSet>>; // key
values are CSSSelector::PseudoType
On 2016/09/01 05:30:22, esprehn wrote:
> Does this need changing in this patch?

No, we can focus on non-zero keys in this patch.

https://codereview.chromium.org/2303443003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/invalidation/InvalidationSet.h (right):

https://codereview.chromium.org/2303443003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/invalidation/InvalidationSet.h:83:
USING_FAST_MALLOC(blink::InvalidationSet);
On 2016/09/01 05:30:22, esprehn wrote:
> Why change this in this patch? The other macro seems about the same, I'm also
> not sure why we annotate InvalidationSet and CSSSelector with
> USING_FAST_MALLOC_WITH_TYPE_NAME but nothing else.

Removed from this patch. I believe the reason is that these have many instances
so having the instrumentation is helpful.

[Eric Willigers, 2016-09-01 14:57:46]

The CQ bit was checked by ericwilligers@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 14:58:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-01 16:34:29]

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

[commit-bot: I haz the power, 2016-09-01 16:34:30]

Dry run: This issue passed the CQ dry run.

[Eric Willigers, 2016-09-02 04:28:21]

ericwilligers@chromium.org changed reviewers:
+ rune@opera.com

[Eric Willigers, 2016-09-02 04:28:22]

Most RuleFeatureSet/InvalidationSet crashes are in set or map operations, for
example
a)  HashTable<AtomicString,KeyValuePair<AtomicString,RefPtr<InvalidationSet>>,
...>::deleteAllBucketsAndDeallocate
b)  ~KeyValuePair<AtomicString,RefPtr<InvalidationSet>>()
c)  HashMap<AtomicString,RefPtr<InvalidationSet>, ...>::clear()
d)  HashTable<AtomicString,AtomicString, ...>::finalize()

This CL will let us know if we are attempting to insert empty strings.

[esprehn, 2016-09-02 09:25:58]

lgtm

[Eric Willigers, 2016-09-02 09:26:39]

The CQ bit was checked by ericwilligers@chromium.org

[commit-bot: I haz the power, 2016-09-02 09:26:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-02 09:33:15]

Description was changed from

==========
CSS: Additional asserts for style invalidation

RuleFeatureSet::add now verifies that we aren't adding to ourselves.

Our hash sets now verify during insertion that our keys are
not empty strings or PseudoUnknown (0).

BUG=412572,640486
==========

to

==========
CSS: Additional asserts for style invalidation

RuleFeatureSet::add now verifies that we aren't adding to ourselves.

Our hash sets now verify during insertion that our keys are
not empty strings or PseudoUnknown (0).

BUG=412572,640486
==========

[commit-bot: I haz the power, 2016-09-02 09:33:16]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-02 09:34:49]

Description was changed from

==========
CSS: Additional asserts for style invalidation

RuleFeatureSet::add now verifies that we aren't adding to ourselves.

Our hash sets now verify during insertion that our keys are
not empty strings or PseudoUnknown (0).

BUG=412572,640486
==========

to

==========
CSS: Additional asserts for style invalidation

RuleFeatureSet::add now verifies that we aren't adding to ourselves.

Our hash sets now verify during insertion that our keys are
not empty strings or PseudoUnknown (0).

BUG=412572,640486

Committed: https://crrev.com/a042e2557c51f8f9f71ea2e00e4ac1016426b242
Cr-Commit-Position: refs/heads/master@{#416220}
==========

[commit-bot: I haz the power, 2016-09-02 09:34:50]

Patchset 2 (id:??) landed as
https://crrev.com/a042e2557c51f8f9f71ea2e00e4ac1016426b242
Cr-Commit-Position: refs/heads/master@{#416220}