Cleanup the SVGTreeScopeResources interface

Cleanup the SVGTreeScopeResources interface

After some previous refactoring, some methods no longer need to be
public, and some methods can be folded into others to avoid some hash-
lookups.
Also use HashMap::removeAll in removeElementFromPendingResources, change
some methods to use references and hide a longish typename with 'auto'.

BUG=454767
Review-Url: https://codereview.chromium.org/2705163008
Cr-Commit-Position: refs/heads/master@{#452645}
Committed: https://chromium.googlesource.com/chromium/src/+/52b947642a5a9fd60c5fbf61419cca26b1fd72ec

[fs, 2017-02-23 11:50:41]

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

[commit-bot: I haz the power, 2017-02-23 11:50:55]

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

[fs, 2017-02-23 11:51:04]

Description was changed from

==========
Cleanup the SVGTreeScopeResources interface

After some previous refactoring, some method's no longer need to be
public, and some methods can be folded into others to avoid some hash-
lookups.
Also use HashMap::removeAll in removeElementFromPendingResources, change
some methods to use references and hide a longish typename with 'auto'.

BUG=454767
==========

to

==========
Cleanup the SVGTreeScopeResources interface

After some previous refactoring, some methods no longer need to be
public, and some methods can be folded into others to avoid some hash-
lookups.
Also use HashMap::removeAll in removeElementFromPendingResources, change
some methods to use references and hide a longish typename with 'auto'.

BUG=454767
==========

[fs, 2017-02-23 13:03:00]

fs@opera.com changed reviewers:
+ pdr@chromium.org, schenney@chromium.org

[commit-bot: I haz the power, 2017-02-23 13:12:51]

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

[commit-bot: I haz the power, 2017-02-23 13:12:52]

Dry run: This issue passed the CQ dry run.

[Stephen Chennney, 2017-02-23 17:00:50]

https://codereview.chromium.org/2705163008/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/SVGTreeScopeResources.cpp (right):

https://codereview.chromium.org/2705163008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/SVGTreeScopeResources.cpp:123:
m_pendingResources.removeAll(toBeRemoved);
From memory this has to come after the clearHasPendingResourcesIfPossible for
use-after-free or some other bug, but maybe I'm wrong. Could you check whether
clearHasPendingResourcesIfPossible can possibly access one of the toBeRemoved
elements? And whether the possible destruction of objects due to
m_pendingResources.removeAll may require that element's pending resources have
been cleared first?

It's probably sufficient to look at change history around this code, going back
at least 4 years. But that might be harder than re-thinking from scratch.

[fs, 2017-02-23 17:10:45]

https://codereview.chromium.org/2705163008/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/SVGTreeScopeResources.cpp (right):

https://codereview.chromium.org/2705163008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/SVGTreeScopeResources.cpp:123:
m_pendingResources.removeAll(toBeRemoved);
On 2017/02/23 at 17:00:50, Stephen Chennney wrote:
> From memory this has to come after the clearHasPendingResourcesIfPossible for
use-after-free or some other bug, but maybe I'm wrong. Could you check whether
clearHasPendingResourcesIfPossible can possibly access one of the toBeRemoved
elements? And whether the possible destruction of objects due to
m_pendingResources.removeAll may require that element's pending resources have
been cleared first?
> 
> It's probably sufficient to look at change history around this code, going
back at least 4 years. But that might be harder than re-thinking from scratch.

This structure is on the GC-heap, so if there're are references still [1]
they'll be kept alive. I can imagine this was quite a different story with
ref-counting though *brrr*.

[1] Theoretically there shouldn't be any, or this method (removeElementFrom...)
has a bug - because this method iterates m_pendingResources and removes
'element' from all the sets it's in, while clearHasPendingResourcesIfPossible
iterates m_pendingResources and clears the bit if 'element' is not in any of the
sets...

[Stephen Chennney, 2017-02-23 17:23:52]

On 2017/02/23 17:10:45, fs wrote:
>
https://codereview.chromium.org/2705163008/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/svg/SVGTreeScopeResources.cpp (right):
> 
>
https://codereview.chromium.org/2705163008/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/svg/SVGTreeScopeResources.cpp:123:
> m_pendingResources.removeAll(toBeRemoved);
> On 2017/02/23 at 17:00:50, Stephen Chennney wrote:
> > From memory this has to come after the clearHasPendingResourcesIfPossible
for
> use-after-free or some other bug, but maybe I'm wrong. Could you check whether
> clearHasPendingResourcesIfPossible can possibly access one of the toBeRemoved
> elements? And whether the possible destruction of objects due to
> m_pendingResources.removeAll may require that element's pending resources have
> been cleared first?
> > 
> > It's probably sufficient to look at change history around this code, going
> back at least 4 years. But that might be harder than re-thinking from scratch.
> 
> This structure is on the GC-heap, so if there're are references still [1]
> they'll be kept alive. I can imagine this was quite a different story with
> ref-counting though *brrr*.
> 
> [1] Theoretically there shouldn't be any, or this method
(removeElementFrom...)
> has a bug - because this method iterates m_pendingResources and removes
> 'element' from all the sets it's in, while clearHasPendingResourcesIfPossible
> iterates m_pendingResources and clears the bit if 'element' is not in any of
the
> sets...

I thought that GC might deal with it, and you're analysis makes sense to me. So
lgtm. Thanks.

[fs, 2017-02-23 17:26:40]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2017-02-23 17:27:07]

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, 2017-02-23 17:36:25]

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

[commit-bot: I haz the power, 2017-02-23 17:36:26]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[fs, 2017-02-23 20:23:26]

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

[commit-bot: I haz the power, 2017-02-23 20:24:06]

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, 2017-02-23 21:47:06]

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

[commit-bot: I haz the power, 2017-02-23 21:47:07]

Dry run: This issue passed the CQ dry run.

[fs, 2017-02-23 21:47:38]

The CQ bit was checked by fs@opera.com

[fs, 2017-02-23 21:47:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
schenney@chromium.org
Link to the patchset: https://codereview.chromium.org/2705163008/#ps20001
(title: "Rebase")

[commit-bot: I haz the power, 2017-02-23 21:48:22]

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, 2017-02-23 21:57:28]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487886458006150,
"parent_rev": "e60c7cdab72dcbf2abdea65ab7ac74ee32f112b4", "commit_rev":
"52b947642a5a9fd60c5fbf61419cca26b1fd72ec"}

[commit-bot: I haz the power, 2017-02-23 21:58:06]

Description was changed from

==========
Cleanup the SVGTreeScopeResources interface

After some previous refactoring, some methods no longer need to be
public, and some methods can be folded into others to avoid some hash-
lookups.
Also use HashMap::removeAll in removeElementFromPendingResources, change
some methods to use references and hide a longish typename with 'auto'.

BUG=454767
==========

to

==========
Cleanup the SVGTreeScopeResources interface

After some previous refactoring, some methods no longer need to be
public, and some methods can be folded into others to avoid some hash-
lookups.
Also use HashMap::removeAll in removeElementFromPendingResources, change
some methods to use references and hide a longish typename with 'auto'.

BUG=454767

Review-Url: https://codereview.chromium.org/2705163008
Cr-Commit-Position: refs/heads/master@{#452645}
Committed:
https://chromium.googlesource.com/chromium/src/+/52b947642a5a9fd60c5fbf61419c...
==========

[commit-bot: I haz the power, 2017-02-23 21:58:08]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/52b947642a5a9fd60c5fbf61419c...