Clear CSSGlobalRuleSet on StyleEngine::didDetach().

Clear CSSGlobalRuleSet on StyleEngine::didDetach().

This could free up memory sooner. Found while investigating 675533, but
not confirmed that this fixes that issue.

BUG=675533
Committed: https://crrev.com/a865f6c19c00f0ba9e021c4871b53d2e115a2b00
Cr-Commit-Position: refs/heads/master@{#439935}

[rune, 2016-12-20 13:18:13]

The CQ bit was checked by rune@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-20 13:18:30]

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-12-20 14:42:12]

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

[commit-bot: I haz the power, 2016-12-20 14:42:13]

Dry run: This issue passed the CQ dry run.

[rune, 2016-12-20 15:02:03]

rune@opera.com changed reviewers:
+ sigbjornf@opera.com

[rune, 2016-12-20 15:02:03]

ptal

[sof, 2016-12-20 18:42:09]

lgtm

Up to you if you think it helps; releasing the underlying RuleFeatureSet's
resources promptly might help a bit.

https://codereview.chromium.org/2589243002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/CSSGlobalRuleSet.cpp (right):

https://codereview.chromium.org/2589243002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/CSSGlobalRuleSet.cpp:72: void
CSSGlobalRuleSet::reset() {
nit: is this leaving the object in a usable state? (..and will it even be used
again by the caller in Q here?) If not, then naming it dispose() would make its
purpose clearer and consistent with how we name these eager-release-of-resources
methods.

[rune, 2016-12-20 18:57:15]

On 2016/12/20 18:42:09, sof wrote:
> lgtm
> 
> Up to you if you think it helps; releasing the underlying RuleFeatureSet's
> resources promptly might help a bit.

Is this stating what this CL does, or is it a proposal for how to improve on it?
(I didn't get it)

>
https://codereview.chromium.org/2589243002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/css/CSSGlobalRuleSet.cpp (right):
> 
>
https://codereview.chromium.org/2589243002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/css/CSSGlobalRuleSet.cpp:72: void
> CSSGlobalRuleSet::reset() {
> nit: is this leaving the object in a usable state? (..and will it even be used
> again by the caller in Q here?) If not, then naming it dispose() would make
its
> purpose clearer and consistent with how we name these
eager-release-of-resources
> methods.

It's leaving it in a usable state, as if it was just constructed, but it won't
be used. I'll change to dispose().

[rune, 2016-12-20 18:57:15]

On 2016/12/20 18:42:09, sof wrote:
> lgtm
> 
> Up to you if you think it helps; releasing the underlying RuleFeatureSet's
> resources promptly might help a bit.

Is this stating what this CL does, or is it a proposal for how to improve on it?
(I didn't get it)

>
https://codereview.chromium.org/2589243002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/css/CSSGlobalRuleSet.cpp (right):
> 
>
https://codereview.chromium.org/2589243002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/css/CSSGlobalRuleSet.cpp:72: void
> CSSGlobalRuleSet::reset() {
> nit: is this leaving the object in a usable state? (..and will it even be used
> again by the caller in Q here?) If not, then naming it dispose() would make
its
> purpose clearer and consistent with how we name these
eager-release-of-resources
> methods.

It's leaving it in a usable state, as if it was just constructed, but it won't
be used. I'll change to dispose().

[sof, 2016-12-20 19:09:46]

On 2016/12/20 18:57:15, rune wrote:
> On 2016/12/20 18:42:09, sof wrote:
> > lgtm
> > 
> > Up to you if you think it helps; releasing the underlying RuleFeatureSet's
> > resources promptly might help a bit.
> 
> Is this stating what this CL does, or is it a proposal for how to improve on
it?
> (I didn't get it)
> 

These eager clearing/disposal operations is something we _can_ add a lot of, but
it's kind of tiresome code to keep around and maintain. So I think we should
trust our memory management infrastructure to release unused allocations
promptly enough, unless we have reason to think not. I am leaving that judgment
here up to you.

[rune, 2016-12-20 20:53:06]

On 2016/12/20 19:09:46, sof wrote:
> On 2016/12/20 18:57:15, rune wrote:
> > On 2016/12/20 18:42:09, sof wrote:
> > > lgtm
> > > 
> > > Up to you if you think it helps; releasing the underlying RuleFeatureSet's
> > > resources promptly might help a bit.
> > 
> > Is this stating what this CL does, or is it a proposal for how to improve on
> it?
> > (I didn't get it)
> > 
> 
> These eager clearing/disposal operations is something we _can_ add a lot of,
but
> it's kind of tiresome code to keep around and maintain. So I think we should
> trust our memory management infrastructure to release unused allocations
> promptly enough, unless we have reason to think not. I am leaving that
judgment
> here up to you.

There is at least one case for which I think this makes sense. I confirmed that
keeping a javascript reference to an iframe document will keep the StyleEngine
alive via Document, yet it is disposed. I've tested that using:

  <!DOCTYPE html>
  <iframe id="frame" src="about:blank"></iframe>
  <script>
      var doc = frame.contentDocument;
      frame.remove();
  </script>

The StyleEngine for the contentDocument is disposed, but not garbage collected
as long as the top level document is active. If I comment out the "var doc"
statement, it is garbage collected as well.

[rune, 2016-12-20 20:53:44]

The CQ bit was checked by rune@opera.com

[rune, 2016-12-20 20:53:44]

The CQ bit was unchecked by rune@opera.com

[rune, 2016-12-20 20:53:44]

The patchset sent to the CQ was uploaded after l-g-t-m from sigbjornf@opera.com
Link to the patchset: https://codereview.chromium.org/2589243002/#ps20001
(title: "reset() -> dispose().")

[rune, 2016-12-20 20:53:45]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2016-12-20 20:54:43]

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-12-20 23:52:17]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1482267225581550,
"parent_rev": "84252b353ecfa7e9e3ec8339adbfb3f22cfaddd1", "commit_rev":
"2db521bca9edc7ccf2154981f04c6a26bedb7868"}

[commit-bot: I haz the power, 2016-12-20 23:52:45]

Description was changed from

==========
Clear CSSGlobalRuleSet on StyleEngine::didDetach().

This could free up memory sooner. Found while investigating 675533, but
not confirmed that this fixes that issue.

BUG=675533
==========

to

==========
Clear CSSGlobalRuleSet on StyleEngine::didDetach().

This could free up memory sooner. Found while investigating 675533, but
not confirmed that this fixes that issue.

BUG=675533

Review-Url: https://codereview.chromium.org/2589243002
==========

[commit-bot: I haz the power, 2016-12-20 23:52:46]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-12-20 23:54:40]

Description was changed from

==========
Clear CSSGlobalRuleSet on StyleEngine::didDetach().

This could free up memory sooner. Found while investigating 675533, but
not confirmed that this fixes that issue.

BUG=675533

Review-Url: https://codereview.chromium.org/2589243002
==========

to

==========
Clear CSSGlobalRuleSet on StyleEngine::didDetach().

This could free up memory sooner. Found while investigating 675533, but
not confirmed that this fixes that issue.

BUG=675533

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

[commit-bot: I haz the power, 2016-12-20 23:54:41]

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