Fix memory error during selector matching due to getMatchedCSSRules.

Fix memory error during selector matching due to getMatchedCSSRules.

The issue is that getMatchedCSSRules wasn't correctly pointing at its
parent stylesheet, which meant that it didn't mark the style resolver
as changed when it's selectorText was updated. This in turn meant that
we had two RuleDatas for a rule that now only had one selector (i.e. it
should only have one RuleData) and we'd try to access the selector at
and index that didn't exist.

Instead, when creating the CSSOM wrappers for getMatchedCSSRules,
find the stylesheet corresponding to each rule and register the wrapper
appropriately with it. We still need to leave in the codepath for
lacking a sheet because we won't file a stylesheet for rules that are
not CSSStyleRules (e.g. @import).

We also leave in the lacking a sheet codepath for the inspector and editing.
I'm not convinced the inspector code is correct, but this patch at
least fixes the error that is accessible from JS. The editing code really
shouldn't be creating CSSOM wrappers.

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

[esprehn, 2013-10-28 23:54:08]

lgtm, but I think you want to move that call to the end of the function.

https://codereview.chromium.org/49093005/diff/1/Source/core/css/ElementRuleCo...
File Source/core/css/ElementRuleCollector.cpp (right):

https://codereview.chromium.org/49093005/diff/1/Source/core/css/ElementRuleCo...
Source/core/css/ElementRuleCollector.cpp:215: sheet->didMutateRules();
I think you're supposed to call didMutateRules at the end of this function call
not at the start like this. You do will*() and then did*() but no mutation
happens in between. That'll make maintaining this harder later.

[ojan, 2013-10-28 23:59:58]

https://codereview.chromium.org/49093005/diff/1/Source/core/css/ElementRuleCo...
File Source/core/css/ElementRuleCollector.cpp (right):

https://codereview.chromium.org/49093005/diff/1/Source/core/css/ElementRuleCo...
Source/core/css/ElementRuleCollector.cpp:215: sheet->didMutateRules();
On 2013/10/28 23:54:08, esprehn wrote:
> I think you're supposed to call didMutateRules at the end of this function
call
> not at the start like this. You do will*() and then did*() but no mutation
> happens in between. That'll make maintaining this harder later.

This function doesn't actually mutate the rules though. That's the sucky thing
about this bit of code. Really this is just getting rid of the stylesheet
sharing.

[commit-bot: I haz the power, 2013-10-29 00:00:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ojan@chromium.org/49093005/1

[commit-bot: I haz the power, 2013-10-29 01:21:41]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2013-10-29 01:27:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ojan@chromium.org/49093005/1

[ojan, 2013-10-29 01:39:37]

Ugh. I'm an idiot. Surprised only one test caught this. didMutateRules
invalidates the StyleRules that we are in the middle of looping through in
ElementRuleCollector::sortAndTransferMatchedRules. I think that's what's going
on anyways.

[ojan, 2013-10-29 01:39:53]

Back to the drawing board on this whole approach.

[ojan, 2013-10-29 19:58:53]

OK. Take 3. Calling willMutateRules/didMutateRules during rule matching is not
safe. Instead, make update the extra CSSOM wrappers inside of willMutateRules to
point to the copies StyleRules.

[ojan, 2013-10-29 19:59:05]

On 2013/10/29 19:58:53, ojan wrote:
> OK. Take 3. Calling willMutateRules/didMutateRules during rule matching is not
> safe. Instead, make update the extra CSSOM wrappers inside of willMutateRules
to
> point to the copies StyleRules.

If not clear, PTAL.

[esprehn, 2013-10-29 20:08:44]

lgtm, I'd return a const ref though instead.

https://codereview.chromium.org/49093005/diff/240001/Source/core/css/CSSStyle...
File Source/core/css/CSSStyleSheet.cpp (right):

https://codereview.chromium.org/49093005/diff/240001/Source/core/css/CSSStyle...
Source/core/css/CSSStyleSheet.cpp:122:
m_extraChildRuleCSSOMWrappers[i]->setParentStyleSheet(0);
braces are not needed.

https://codereview.chromium.org/49093005/diff/240001/Source/core/css/CSSStyle...
Source/core/css/CSSStyleSheet.cpp:132:
indices.grow(m_extraChildRuleCSSOMWrappers.size());
Instead you can make this function return a const Vector<unsigned>& and just
return indicies at the end.

https://codereview.chromium.org/49093005/diff/240001/Source/core/css/CSSStyle...
Source/core/css/CSSStyleSheet.cpp:164: extraCSSOMWrapperIndices(indices);
const Vector<unsigned>& indexes = cssomWrapperIndexes();

https://codereview.chromium.org/49093005/diff/240001/Source/core/css/CSSStyle...
Source/core/css/CSSStyleSheet.cpp:206:
m_extraChildRuleCSSOMWrappers[i]->reattach(m_contents->ruleAt(extraCSSOMWrapperIndices[i]));
extra braces

[ojan, 2013-10-29 20:35:14]

https://codereview.chromium.org/49093005/diff/240001/Source/core/css/CSSStyle...
File Source/core/css/CSSStyleSheet.cpp (right):

https://codereview.chromium.org/49093005/diff/240001/Source/core/css/CSSStyle...
Source/core/css/CSSStyleSheet.cpp:122:
m_extraChildRuleCSSOMWrappers[i]->setParentStyleSheet(0);
On 2013/10/29 20:08:44, esprehn wrote:
> braces are not needed.

Done.

https://codereview.chromium.org/49093005/diff/240001/Source/core/css/CSSStyle...
Source/core/css/CSSStyleSheet.cpp:132:
indices.grow(m_extraChildRuleCSSOMWrappers.size());
On 2013/10/29 20:08:44, esprehn wrote:
> Instead you can make this function return a const Vector<unsigned>& and just
> return indicies at the end.

As discussed offline, this doesn't work.

https://codereview.chromium.org/49093005/diff/240001/Source/core/css/CSSStyle...
Source/core/css/CSSStyleSheet.cpp:206:
m_extraChildRuleCSSOMWrappers[i]->reattach(m_contents->ruleAt(extraCSSOMWrapperIndices[i]));
On 2013/10/29 20:08:44, esprehn wrote:
> extra braces

Done.

[commit-bot: I haz the power, 2013-10-29 20:53:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ojan@chromium.org/49093005/310001

[commit-bot: I haz the power, 2013-10-29 23:56:40]

Change committed as 160879