Bug 15451 – SVGStyledElement must unregister itself from Resources on detach

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Eric Seidel (no email)]

SVGStyledElement must unregister itself from Resources on detach

Right now an SVGStyledElement will add itself as a client to resources when it paints with them (so it can be told when to invalidate itself).  but it never ever unregisters.  Even if it's deleted.  This means crashing.

We could make a fancy system to watch the ids and unregister if the filterId changes, or whatever, but a simpler solution is to just keep a hash on the document pointing from elements to sets of resources, and then remove that elemetn from those resources when that element is destroyed.

The Resource would add the element to this hash in SVGResource::addClient if the the resource doesn't already have that element in its client HashSet.

Oliver said he would work on a fix.

[Eric Seidel (no email)]

Created attachment 16611 [details]
test case (will crash after a while)

The test case could probably be made smaller (certainly for DRT where you can manually force a GC collection).

[Eric Seidel (no email)]

Reproducible crashers are P1s.  This affects both feature-branch and trunk.

[Oliver Hunt]

Created attachment 16612 [details]
Initial run at fixing this

Comment on attachment 16612 [details]
Initial run at fixing this

I can't review, but:

+        WARNING: NO TEST CASES ADDED OR CHANGED

Is there no way to make a test case that crashes DRT or misbehaves? Even a test that only crashes under guardmalloc is better than no test at all.

+static HashMap<SVGStyledElement*, SVGResource*>& clientMapForResource(SVGResource* resource) {    

The brace should go on the next line.

[Eric Seidel (no email)]

Comment on attachment 16612 [details]
Initial run at fixing this

I'm not sure you really need 4 separate maps.  One resource map should probably be enough.  Needs a test case.  Otherwise looks good.

[Oliver Hunt]

Created attachment 16620 [details]
Fix attempt #2, a single hashtable

[Eric Seidel (no email)]

Comment on attachment 16620 [details]
Fix attempt #2, a single hashtable

+    {
+        for (int i = 0; i < _ResourceTypeCount; i++)
+            resources[i] = 0;
+    }

could also just be a call to memset... but that's fine.

Removing the 0 from +        ClipperResourceType,
 could make _ResourceTypeCount be not the right number on some platforms (but maybe K&R says otherwise..)

I thinik this is the only place I've seen _ used in an enum before.  Mayabe it should be LastResourceType like is used by CSS values or ResourceTypeMax

Why the var name "item" (twice).  That seems like a hold-over from the KCanvas days.

I think that addClient's lookup should just be a get:
+    HashMap<SVGStyledElement*, ResourceSet*>::iterator associatedResource = clientMap().find(item);

Then you don't have to compare against the end iterator, you just check if (target) and if !target then you set it to a new ResourceSet.  seems cleaner.

SVGResource doesn't know how to remove itself from ResourceSet when it dies.  addClient's line:

+    if (SVGResource* oldResource = target->resources[type])
+        oldResource->m_clients.remove(item);

will crash w/o fixing that.

[Oliver Hunt]

Created attachment 16622 [details]
Again, this time handling the SVGResource destruction as well

[Eric Seidel (no email)]

Comment on attachment 16622 [details]
Again, this time handling the SVGResource destruction as well

Style guidelines:
+    ResourceSet() 
+    {

 SVGResource::~SVGResource()
is a bit too clever for my tastes.  

If you break it into two for loops (you can even use the same itr variable) it would be easier to understand. (I think).

I would still change "item" to "element" or something more appropriate now that these things aren't KCanvasItems anymore (the old name for RenderPath.. back when the renderers used to be clients directly).

I dont' need to see this again, but please do those small fixes when you land.

[Eric Seidel (no email)]

Landed as r26351 on feature-branch.