Improve handling of duplicate id's for SVG resources

Improve handling of duplicate id's for SVG resources

This CL attempts to fix the known issues with duplicate id's and SVG
resources. There are a variety of cases where this fails, and the added
tests attempt to cover those as well as possible.

The know bugs generally stem from:

 * Resources being registered in (layout) tree order after style
   recalc. This for instance mean that any later defined resources
   will shadow any earlier appearing resource (which would the correct
   one.)

 * Removing a resource container does not consider that there could now
   be another resource that is no longer shadowed by the one removed.
   Together with the above, this also meant that removing a resource
   from the DOM could invalidate, and break, all occurences of said
   resource.

This CL attempts to handle the above by factoring the result of
getElementById into the equation, considering it to be "the truth" when
possible/required. The new methods registerResource and
unregisterResource form the basis of this, codifying the two basic
operations on which the tracking is built. The tracking of the
'registered' flag from LayoutSVGResourceContainer is now handled by
SVGTreeScopeResources. While this flag could be considered an
optimization at this point, DCHECKs are added to attempt to keep it
true to it's purpose.

BUG=454767
Review-Url: https://codereview.chromium.org/2722543002
Cr-Commit-Position: refs/heads/master@{#454951}
Committed: https://chromium.googlesource.com/chromium/src/+/533d683571c860e69bb3ecaf6214c5ceed25a808

[fs, 2017-02-27 12:45:47]

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

[commit-bot: I haz the power, 2017-02-27 12:45:51]

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-27 14:24:47]

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

[commit-bot: I haz the power, 2017-02-27 14:24:48]

Dry run: This issue passed the CQ dry run.

[fs, 2017-02-27 14:34:36]

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

[fs, 2017-02-27 14:34:37]

So this fixes things up within the current framework. I think we should attempt
to transition away from keeping references to LayoutObjects though, maybe in
steps like:

struct ElementReference {
  Member<Element> target;
  HashSet<LayoutObject*> clients;
  HeapHashSet<Member<Element>> pendingClients;
};

and then:

struct ElementReference {
  Member<Element> target;
  HeapHashSet<Member<SVGResourceClient>> clients;
  HeapHashSet<Member<Element>> pendingClients;
};

Hopefully that will allow itself to be done incrementally...

Also, please suggest further cases to test if you see any...

[Stephen Chennney, 2017-02-27 16:21:22]

This might be a good time to add comments like crazy. I had a tough time
following along, so please correct me if I seem to be misunderstanding
something.

https://codereview.chromium.org/2722543002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/svg/custom/duplicate-ids-addition.html
(right):

https://codereview.chromium.org/2722543002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/svg/custom/duplicate-ids-addition.html:18:
document.querySelector('svg').insertBefore(gradient, rect);
Another test could be done, where the added element is the one that should
become the new resource. So insert a new one before the existing gradient.

https://codereview.chromium.org/2722543002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/svg/custom/duplicate-ids-addition.html:20:
rect.setAttribute('stroke-width', 0);
Why do you do this? Invalidation bug?

https://codereview.chromium.org/2722543002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/svg/custom/duplicate-ids-removal.html
(right):

https://codereview.chromium.org/2722543002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/svg/custom/duplicate-ids-removal.html:14:
document.querySelectorAll('lineargradient')[1].remove();
Another test comes to mind. If you remove the first one we should be sure to
recompute and pick up the second one (which will now be the first one).

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

https://codereview.chromium.org/2722543002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/SVGTreeScopeResources.cpp:42: if
(resource->isRegistered() || id.isEmpty())
This check is the only use of "resource" and I find it confusing to pass it into
this method just for that purpose. As I understand it, the passed resource is
the old resource for that id and if it's registered then it's still the right
one to use so we shouldn't update. Is that right?

Could this if be moved to the caller? Or maybe rename either the method or the
argument or both to make it clear what's going on? e.g. updateResourceIfNeeded?

[fs, 2017-02-27 17:47:20]

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

[fs, 2017-02-27 17:47:33]

https://codereview.chromium.org/2722543002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/svg/custom/duplicate-ids-addition.html
(right):

https://codereview.chromium.org/2722543002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/svg/custom/duplicate-ids-addition.html:18:
document.querySelector('svg').insertBefore(gradient, rect);
On 2017/02/27 at 16:21:22, Stephen Chennney wrote:
> Another test could be done, where the added element is the one that should
become the new resource. So insert a new one before the existing gradient.

Done.

https://codereview.chromium.org/2722543002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/svg/custom/duplicate-ids-addition.html:20:
rect.setAttribute('stroke-width', 0);
On 2017/02/27 at 16:21:22, Stephen Chennney wrote:
> Why do you do this? Invalidation bug?

This is here to trigger style-recalc. (Needed to make this fail on current
master. AKA: "pushing the right buttons")

https://codereview.chromium.org/2722543002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/svg/custom/duplicate-ids-removal.html
(right):

https://codereview.chromium.org/2722543002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/svg/custom/duplicate-ids-removal.html:14:
document.querySelectorAll('lineargradient')[1].remove();
On 2017/02/27 at 16:21:22, Stephen Chennney wrote:
> Another test comes to mind. If you remove the first one we should be sure to
recompute and pick up the second one (which will now be the first one).

I wrote this test, but it's currently failing (so nice catch!) so I need to
debug this some more. (I suspect it might be related to the "currentResource ==
resource" check in removeResource, which would be "doh!" I suspect...)

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

https://codereview.chromium.org/2722543002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/SVGTreeScopeResources.cpp:42: if
(resource->isRegistered() || id.isEmpty())
On 2017/02/27 at 16:21:22, Stephen Chennney wrote:
> This check is the only use of "resource" and I find it confusing to pass it
into this method just for that purpose. As I understand it, the passed resource
is the old resource for that id and if it's registered then it's still the right
one to use so we shouldn't update. Is that right?

It may or may not be the old resource. What we know is that it's _a_resource_
with the passed 'id'. As stated in the description, the flag-check is an
optimization.

> Could this if be moved to the caller? Or maybe rename either the method or the
argument or both to make it clear what's going on? e.g. updateResourceIfNeeded?

It could. The reason for having it here is to have all the flag mongering in one
place, and make LayoutSVGResourceContainer not care about it at all. In the
future "registering" would be made implicit and the flag dropped entirely.

[commit-bot: I haz the power, 2017-02-27 17:47:48]

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-27 19:11:25]

https://codereview.chromium.org/2722543002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/svg/custom/duplicate-ids-removal.html
(right):

https://codereview.chromium.org/2722543002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/svg/custom/duplicate-ids-removal.html:14:
document.querySelectorAll('lineargradient')[1].remove();
On 2017/02/27 at 17:47:30, fs wrote:
> On 2017/02/27 at 16:21:22, Stephen Chennney wrote:
> > Another test comes to mind. If you remove the first one we should be sure to
recompute and pick up the second one (which will now be the first one).
> 
> I wrote this test, but it's currently failing (so nice catch!) so I need to
debug this some more. (I suspect it might be related to the "currentResource ==
resource" check in removeResource, which would be "doh!" I suspect...)

I realize that that test likely worked "by accident" before because the second
gradient would be the one in the map after style-recalc/layout. (Easily observed
when running the test because it does flash red -> green...) I'll see if can do
something about without adding too much complexity though...

[Stephen Chennney, 2017-02-27 19:31:29]

On 2017/02/27 19:11:25, fs wrote:
>
https://codereview.chromium.org/2722543002/diff/1/third_party/WebKit/LayoutTe...
> File third_party/WebKit/LayoutTests/svg/custom/duplicate-ids-removal.html
> (right):
> 
>
https://codereview.chromium.org/2722543002/diff/1/third_party/WebKit/LayoutTe...
> third_party/WebKit/LayoutTests/svg/custom/duplicate-ids-removal.html:14:
> document.querySelectorAll('lineargradient')[1].remove();
> On 2017/02/27 at 17:47:30, fs wrote:
> > On 2017/02/27 at 16:21:22, Stephen Chennney wrote:
> > > Another test comes to mind. If you remove the first one we should be sure
to
> recompute and pick up the second one (which will now be the first one).
> > 
> > I wrote this test, but it's currently failing (so nice catch!) so I need to
> debug this some more. (I suspect it might be related to the "currentResource
==
> resource" check in removeResource, which would be "doh!" I suspect...)
> 
> I realize that that test likely worked "by accident" before because the second
> gradient would be the one in the map after style-recalc/layout. (Easily
observed
> when running the test because it does flash red -> green...) I'll see if can
do
> something about without adding too much complexity though...

Would you like me to wait for more changes before I review again?

[commit-bot: I haz the power, 2017-02-27 19:34:09]

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

[commit-bot: I haz the power, 2017-02-27 19:34:09]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[fs, 2017-02-27 20:01:15]

On 2017/02/27 at 19:31:29, schenney wrote:
> On 2017/02/27 19:11:25, fs wrote:
> >
https://codereview.chromium.org/2722543002/diff/1/third_party/WebKit/LayoutTe...
> > File third_party/WebKit/LayoutTests/svg/custom/duplicate-ids-removal.html
> > (right):
> > 
> >
https://codereview.chromium.org/2722543002/diff/1/third_party/WebKit/LayoutTe...
> > third_party/WebKit/LayoutTests/svg/custom/duplicate-ids-removal.html:14:
> > document.querySelectorAll('lineargradient')[1].remove();
> > On 2017/02/27 at 17:47:30, fs wrote:
> > > On 2017/02/27 at 16:21:22, Stephen Chennney wrote:
> > > > Another test comes to mind. If you remove the first one we should be
sure to
> > recompute and pick up the second one (which will now be the first one).
> > > 
> > > I wrote this test, but it's currently failing (so nice catch!) so I need
to
> > debug this some more. (I suspect it might be related to the "currentResource
==
> > resource" check in removeResource, which would be "doh!" I suspect...)
> > 
> > I realize that that test likely worked "by accident" before because the
second
> > gradient would be the one in the map after style-recalc/layout. (Easily
observed
> > when running the test because it does flash red -> green...) I'll see if can
do
> > something about without adding too much complexity though...
> 
> Would you like me to wait for more changes before I review again?

I don't think that should be necessary. (And I'm not planning on doing any more
work on it today.) We might want to decide what we think about the issue above
though.

[Stephen Chennney, 2017-02-27 20:16:36]

On 2017/02/27 20:01:15, fs wrote:
> On 2017/02/27 at 19:31:29, schenney wrote:
> > On 2017/02/27 19:11:25, fs wrote:
> > >
>
https://codereview.chromium.org/2722543002/diff/1/third_party/WebKit/LayoutTe...
> > > File third_party/WebKit/LayoutTests/svg/custom/duplicate-ids-removal.html
> > > (right):
> > > 
> > >
>
https://codereview.chromium.org/2722543002/diff/1/third_party/WebKit/LayoutTe...
> > > third_party/WebKit/LayoutTests/svg/custom/duplicate-ids-removal.html:14:
> > > document.querySelectorAll('lineargradient')[1].remove();
> > > On 2017/02/27 at 17:47:30, fs wrote:
> > > > On 2017/02/27 at 16:21:22, Stephen Chennney wrote:
> > > > > Another test comes to mind. If you remove the first one we should be
> sure to
> > > recompute and pick up the second one (which will now be the first one).
> > > > 
> > > > I wrote this test, but it's currently failing (so nice catch!) so I need
> to
> > > debug this some more. (I suspect it might be related to the
"currentResource
> ==
> > > resource" check in removeResource, which would be "doh!" I suspect...)
> > > 
> > > I realize that that test likely worked "by accident" before because the
> second
> > > gradient would be the one in the map after style-recalc/layout. (Easily
> observed
> > > when running the test because it does flash red -> green...) I'll see if
can
> do
> > > something about without adding too much complexity though...
> > 
> > Would you like me to wait for more changes before I review again?
> 
> I don't think that should be necessary. (And I'm not planning on doing any
more
> work on it today.) We might want to decide what we think about the issue above
> though.

This gets us to a much better place, even if we regress something we previously
just got lucky on. I'm in favor of taking 5 steps forward with one step
backward. As is it's lgtm from me, but you'll need to decide whether to add an
expectation for the failing test or fix it. If you fix it then I'll need to look
again. (I'm leaving the choice up to you so that I don't block you tomorrow
morning).

[fs, 2017-02-27 20:18:35]

On 2017/02/27 at 20:16:36, schenney wrote:
> On 2017/02/27 20:01:15, fs wrote:
> > On 2017/02/27 at 19:31:29, schenney wrote:
> > > On 2017/02/27 19:11:25, fs wrote:
> > > >
> >
https://codereview.chromium.org/2722543002/diff/1/third_party/WebKit/LayoutTe...
> > > > File
third_party/WebKit/LayoutTests/svg/custom/duplicate-ids-removal.html
> > > > (right):
> > > > 
> > > >
> >
https://codereview.chromium.org/2722543002/diff/1/third_party/WebKit/LayoutTe...
> > > > third_party/WebKit/LayoutTests/svg/custom/duplicate-ids-removal.html:14:
> > > > document.querySelectorAll('lineargradient')[1].remove();
> > > > On 2017/02/27 at 17:47:30, fs wrote:
> > > > > On 2017/02/27 at 16:21:22, Stephen Chennney wrote:
> > > > > > Another test comes to mind. If you remove the first one we should be
> > sure to
> > > > recompute and pick up the second one (which will now be the first one).
> > > > > 
> > > > > I wrote this test, but it's currently failing (so nice catch!) so I
need
> > to
> > > > debug this some more. (I suspect it might be related to the
"currentResource
> > ==
> > > > resource" check in removeResource, which would be "doh!" I suspect...)
> > > > 
> > > > I realize that that test likely worked "by accident" before because the
> > second
> > > > gradient would be the one in the map after style-recalc/layout. (Easily
> > observed
> > > > when running the test because it does flash red -> green...) I'll see if
can
> > do
> > > > something about without adding too much complexity though...
> > > 
> > > Would you like me to wait for more changes before I review again?
> > 
> > I don't think that should be necessary. (And I'm not planning on doing any
more
> > work on it today.) We might want to decide what we think about the issue
above
> > though.
> 
> This gets us to a much better place, even if we regress something we
previously just got lucky on. I'm in favor of taking 5 steps forward with one
step backward. As is it's lgtm from me, but you'll need to decide whether to add
an expectation for the failing test or fix it. If you fix it then I'll need to
look again. (I'm leaving the choice up to you so that I don't block you tomorrow
morning).

If so, I think I'll just remove the test (for now, and commit together with a
fix.)

[Stephen Chennney, 2017-02-27 20:27:32]

On 2017/02/27 20:18:35, fs wrote:
> On 2017/02/27 at 20:16:36, schenney wrote:
> > On 2017/02/27 20:01:15, fs wrote:
> > > On 2017/02/27 at 19:31:29, schenney wrote:
> > > > On 2017/02/27 19:11:25, fs wrote:
> > > > >
> > >
>
https://codereview.chromium.org/2722543002/diff/1/third_party/WebKit/LayoutTe...
> > > > > File
> third_party/WebKit/LayoutTests/svg/custom/duplicate-ids-removal.html
> > > > > (right):
> > > > > 
> > > > >
> > >
>
https://codereview.chromium.org/2722543002/diff/1/third_party/WebKit/LayoutTe...
> > > > >
third_party/WebKit/LayoutTests/svg/custom/duplicate-ids-removal.html:14:
> > > > > document.querySelectorAll('lineargradient')[1].remove();
> > > > > On 2017/02/27 at 17:47:30, fs wrote:
> > > > > > On 2017/02/27 at 16:21:22, Stephen Chennney wrote:
> > > > > > > Another test comes to mind. If you remove the first one we should
be
> > > sure to
> > > > > recompute and pick up the second one (which will now be the first
one).
> > > > > > 
> > > > > > I wrote this test, but it's currently failing (so nice catch!) so I
> need
> > > to
> > > > > debug this some more. (I suspect it might be related to the
> "currentResource
> > > ==
> > > > > resource" check in removeResource, which would be "doh!" I suspect...)
> > > > > 
> > > > > I realize that that test likely worked "by accident" before because
the
> > > second
> > > > > gradient would be the one in the map after style-recalc/layout.
(Easily
> > > observed
> > > > > when running the test because it does flash red -> green...) I'll see
if
> can
> > > do
> > > > > something about without adding too much complexity though...
> > > > 
> > > > Would you like me to wait for more changes before I review again?
> > > 
> > > I don't think that should be necessary. (And I'm not planning on doing any
> more
> > > work on it today.) We might want to decide what we think about the issue
> above
> > > though.
> > 
> > This gets us to a much better place, even if we regress something we
> previously just got lucky on. I'm in favor of taking 5 steps forward with one
> step backward. As is it's lgtm from me, but you'll need to decide whether to
add
> an expectation for the failing test or fix it. If you fix it then I'll need to
> look again. (I'm leaving the choice up to you so that I don't block you
tomorrow
> morning).
> 
> If so, I think I'll just remove the test (for now, and commit together with a
> fix.)

Go for it. Works for me. Huge thanks for finally solving this.

[pdr., 2017-02-27 23:06:31]

LGTM 2

https://codereview.chromium.org/2722543002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceContainer.h
(right):

https://codereview.chromium.org/2722543002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceContainer.h:68: void
setRegistered(bool registered) { m_registered = registered; }
Nit: to prevent calling this at the wrong time, could you make this private and
friend SVGTreeScopeResources? Also, can you add a comment here about what it
means to register a resource?

https://codereview.chromium.org/2722543002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGTreeScopeResources.cpp (right):

https://codereview.chromium.org/2722543002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGTreeScopeResources.cpp:27: static
LayoutSVGResourceContainer* lookupResource(TreeScope& treeScope,
Should this lookup ever fail? I am wondering if we can return a reference and
add CHECKs that element and layoutObject are not null.

[fs, 2017-02-28 09:26:26]

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

[commit-bot: I haz the power, 2017-02-28 09:26:40]

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-28 12:54:39]

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

[commit-bot: I haz the power, 2017-02-28 12:54:41]

Dry run: This issue passed the CQ dry run.

[fs, 2017-03-06 17:10:38]

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

[commit-bot: I haz the power, 2017-03-06 17:10:53]

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-03-06 20:38:39]

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

[commit-bot: I haz the power, 2017-03-06 20:38:40]

Dry run: This issue passed the CQ dry run.

[fs, 2017-03-06 20:49:55]

Since the M58 branch has been cut, I'll go ahead and land this now, with the
intention of transitioning the whole thing to something better before the next
branch.

(meh, forgot to publish the following old replies...)

https://codereview.chromium.org/2722543002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceContainer.h
(right):

https://codereview.chromium.org/2722543002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceContainer.h:68: void
setRegistered(bool registered) { m_registered = registered; }
On 2017/02/27 at 23:06:30, pdr. wrote:
> Nit: to prevent calling this at the wrong time, could you make this private
and friend SVGTreeScopeResources? Also, can you add a comment here about what it
means to register a resource?

Done.

https://codereview.chromium.org/2722543002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGTreeScopeResources.cpp (right):

https://codereview.chromium.org/2722543002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGTreeScopeResources.cpp:27: static
LayoutSVGResourceContainer* lookupResource(TreeScope& treeScope,
On 2017/02/27 at 23:06:30, pdr. wrote:
> Should this lookup ever fail? I am wondering if we can return a reference and
add CHECKs that element and layoutObject are not null.

Besides the LayoutObject type-check, it's expected that removing the last
element with a certain 'id' will make gEBI return nullptr. There's also the case
where the element with the 'id' for some reason doesn't have a LayoutObject
(stray resource, random element with id w/ display: none, et.c.)

[fs, 2017-03-06 20:50:18]

The CQ bit was checked by fs@opera.com

[fs, 2017-03-06 20:50:18]

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

[commit-bot: I haz the power, 2017-03-06 20:50:42]

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-03-06 20:56:10]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1488833418294190,
"parent_rev": "ed17a3d795edc5af91cd6074a7c299425b1fe9d7", "commit_rev":
"533d683571c860e69bb3ecaf6214c5ceed25a808"}

[commit-bot: I haz the power, 2017-03-06 20:56:52]

Description was changed from

==========
Improve handling of duplicate id's for SVG resources

This CL attempts to fix the known issues with duplicate id's and SVG
resources. There are a variety of cases where this fails, and the added
tests attempt to cover those as well as possible.

The know bugs generally stem from:

 * Resources being registered in (layout) tree order after style
   recalc. This for instance mean that any later defined resources
   will shadow any earlier appearing resource (which would the correct
   one.)

 * Removing a resource container does not consider that there could now
   be another resource that is no longer shadowed by the one removed.
   Together with the above, this also meant that removing a resource
   from the DOM could invalidate, and break, all occurences of said
   resource.

This CL attempts to handle the above by factoring the result of
getElementById into the equation, considering it to be "the truth" when
possible/required. The new methods registerResource and
unregisterResource form the basis of this, codifying the two basic
operations on which the tracking is built. The tracking of the
'registered' flag from LayoutSVGResourceContainer is now handled by
SVGTreeScopeResources. While this flag could be considered an
optimization at this point, DCHECKs are added to attempt to keep it
true to it's purpose.

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

to

==========
Improve handling of duplicate id's for SVG resources

This CL attempts to fix the known issues with duplicate id's and SVG
resources. There are a variety of cases where this fails, and the added
tests attempt to cover those as well as possible.

The know bugs generally stem from:

 * Resources being registered in (layout) tree order after style
   recalc. This for instance mean that any later defined resources
   will shadow any earlier appearing resource (which would the correct
   one.)

 * Removing a resource container does not consider that there could now
   be another resource that is no longer shadowed by the one removed.
   Together with the above, this also meant that removing a resource
   from the DOM could invalidate, and break, all occurences of said
   resource.

This CL attempts to handle the above by factoring the result of
getElementById into the equation, considering it to be "the truth" when
possible/required. The new methods registerResource and
unregisterResource form the basis of this, codifying the two basic
operations on which the tracking is built. The tracking of the
'registered' flag from LayoutSVGResourceContainer is now handled by
SVGTreeScopeResources. While this flag could be considered an
optimization at this point, DCHECKs are added to attempt to keep it
true to it's purpose.

BUG=454767

Review-Url: https://codereview.chromium.org/2722543002
Cr-Commit-Position: refs/heads/master@{#454951}
Committed:
https://chromium.googlesource.com/chromium/src/+/533d683571c860e69bb3ecaf6214...
==========

[commit-bot: I haz the power, 2017-03-06 20:56:54]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/533d683571c860e69bb3ecaf6214...