Bug 46791 - [PATCH] NullPointerException calling cloneNode for SVG 1.2 documents
[PATCH] NullPointerException calling cloneNode for SVG 1.2 documents
Status: NEW
Product: Batik - Now in Jira
Classification: Unclassified
Component: SVG DOM
1.8
All All
: P2 normal
: ---
Assigned To: Batik Developer's Mailing list
http://www.nabble.com/NPE-calling-clo...
: PatchAvailable
Depends on:
Blocks: 50045
  Show dependency tree
 
Reported: 2009-03-02 23:54 UTC by Daniel Westheide
Modified: 2010-10-06 05:37 UTC (History)
2 users (show)



Attachments
A patch that fixes the described bug. (2.14 KB, patch)
2009-03-02 23:54 UTC, Daniel Westheide
Details | Diff
Test case (SVG 1.2) - this raises the exception (433 bytes, image/svg+xml)
2009-03-03 01:58 UTC, Helder Magalhães
Details
Test case (SVG 1.1) - this does *not* raise the exception (only for comparison purposes) (419 bytes, image/svg+xml)
2009-03-03 02:00 UTC, Helder Magalhães
Details
Patch for the bug with correct code formatting. (1.81 KB, patch)
2009-03-04 04:46 UTC, Daniel Westheide
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Westheide 2009-03-02 23:54:32 UTC
Created attachment 23321 [details]
A patch that fixes the described bug.

If you call the Node#cloneNode(boolean) method to perform a deep clone of an element in an SVG 1.2 document, and that element has got a child element in a custom namespace, a NullPointerException is thrown:

Exception in thread "main" java.lang.NullPointerException
	at org.apache.batik.dom.svg.SVGOMElement.createLiveAnimatedString(SVGOMElement.java:390)
	at org.apache.batik.dom.svg.SVGStylableElement.initializeLiveAttributes(SVGStylableElement.java:118)
	at org.apache.batik.dom.svg.SVGStylableElement.<init>(SVGStylableElement.java:103)
	at org.apache.batik.dom.svg.SVGGraphicsElement.<init>(SVGGraphicsElement.java:96)
	at org.apache.batik.dom.svg12.BindableElement.<init>(BindableElement.java:68)
	at org.apache.batik.dom.svg12.BindableElement.newNode(BindableElement.java:99)
	at org.apache.batik.dom.AbstractNode.cloneNode(AbstractNode.java:309)
	at org.apache.batik.dom.AbstractParentNode.deepCopyInto(AbstractParentNode.java:377)
	at org.apache.batik.dom.AbstractElement.deepCopyInto(AbstractElement.java:563)
	at org.apache.batik.dom.svg.SVGOMElement.deepCopyInto(SVGOMElement.java:891)
	at org.apache.batik.dom.AbstractNode.cloneNode(AbstractNode.java:309)
	at org.apache.batik.dom.AbstractParentNode.deepCopyInto(AbstractParentNode.java:377)
	at org.apache.batik.dom.AbstractElement.deepCopyInto(AbstractElement.java:563)
	at org.apache.batik.dom.svg.SVGOMElement.deepCopyInto(SVGOMElement.java:891)
	at org.apache.batik.dom.AbstractNode.cloneNode(AbstractNode.java:309)
	at org.apache.batik.dom.AbstractParentNode.deepCopyInto(AbstractParentNode.java:377)
	at org.apache.batik.dom.AbstractElement.deepCopyInto(AbstractElement.java:563)
	at org.apache.batik.dom.svg.SVGOMElement.deepCopyInto(SVGOMElement.java:891)
	at org.apache.batik.dom.AbstractNode.cloneNode(AbstractNode.java:309)
	at CloneNodeTest.main(CloneNodeTest.java:25)

In SVG 1.1 documents, this does not happen.

The problem is that the cloning behaviour of BindableElement is implemented incorrectly. The newNode() method calls the wrong constructor, so that some super constructor executes code relying on the owner document of the node aready being set.

Attached is a patch for this bug. The cloning behaviour is now implemented just as the one of GenericElementNS, the class used for custom namespace elements in SVG 1.1. This means that in the newNode() method, we just call the parameterless constructor. To ensure that the fields of BindableElement are set, I added implementations for the copyInto, deepCopyInto, export, and deepExport methods.
Comment 1 Helder Magalhães 2009-03-03 01:37:12 UTC
Hi Daniel,

Nice to see your contribution. ;-)

Without much knowledge on Batik's internals, I'm only reviewing the patch in terms of coding principles (at least for now).


>  import org.apache.batik.dom.svg.SVGGraphicsElement;
> -
>  import org.w3c.dom.Node;

This apparently redundant line separating (java from batik from w3c) includes is expected. See, for example, "sources/org/apache/batik/bridge/BaseScriptingEnvironment.java".


>      protected Node newNode() {
> -        return new BindableElement(null, null, namespaceURI, localName);
> +    	return new BindableElement();

Indenting issue, please use 4 spaces for consistency.


> -    /**
> +	/**

Indenting issue, please use 4 spaces instead of tab character.


By the way, patches can be acknowledged by adding the "PatchAvailable" keyword to the report (which I've done). Adding the word to the report's title is also commonly seen, and I believe using both won't hurt (although I can be wrong). ;-)


Finally, I'm not sure if the patch should include a modification to the "CHANGES" file or if this is to be addressed by whoever commits it (although I'd assume the latter).


Regards,
 Helder Magalhães
Comment 2 Helder Magalhães 2009-03-03 01:58:15 UTC
Created attachment 23322 [details]
Test case (SVG 1.2) - this raises the exception

Reduced test case which reproduces the issue. By removing the version property (version="1.2") the document is set into SVG 1.1 mode and the exception is avoided.
Comment 3 Helder Magalhães 2009-03-03 02:00:07 UTC
Created attachment 23323 [details]
Test case (SVG 1.1) - this does *not* raise the exception (only for comparison purposes)
Comment 4 Daniel Westheide 2009-03-03 02:43:55 UTC
Hi Helder,

thanks for your help and for providing the two test cases. As for your review of the patch, I was not aware of that and trusted my IDE all too much. :-) In the future, I will take care of that.

Should I update this patch based on your review or is that okay for now?
Comment 5 Thomas Deweese 2009-03-03 02:58:50 UTC
I have a question on the patch.

The main effect of the patch is to bypass the
baseclass call to 'initializeAllLiveAttributes'
the question I have is when does that get called?
If it isn't ever called then Animated attributes won't work.

On that point, Cameron - should the initialization of 
the live attributes be tied to the booting of the SVG
and CSS DOM?
Comment 6 Daniel Westheide 2009-03-03 03:10:46 UTC
Hi Thomas,

the initializeAllLiveAttributes method is still called by the deepCopyInto method of SVGOMElement. When the newNode() method of BindableElement used the parameterized constructor, the initializeAllLiveAttributes method would actually have been called twice, if that first call had not caused the NPE due to the owner document not being set yet.
Comment 7 Cameron McCormack 2009-03-03 16:08:21 UTC
(In reply to comment #5)
> On that point, Cameron - should the initialization of 
> the live attributes be tied to the booting of the SVG
> and CSS DOM?

That would be better, since you could then avoid constructing the various SVGLength, etc. objects which wouldn't be useful without the SVG DOM being initialised anyway.

Before animation was implemented, these objects were lazily created.  So it'd be good to go back to something like that.

(In reply to comment #6)
> the initializeAllLiveAttributes method is still called by the deepCopyInto
> method of SVGOMElement. When the newNode() method of BindableElement used the
> parameterized constructor, the initializeAllLiveAttributes method would
> actually have been called twice, if that first call had not caused the NPE due
> to the owner document not being set yet.

I only took a very quick look, and I think Daniel is correct.  The pattern of overloading those four methods to ensure some extra data is copied across to the cloned node is also used in SVGOMElement.
Comment 8 Helder Magalhães 2009-03-04 01:55:59 UTC
(In reply to comment #4)
> Should I update this patch based on your review or is that okay for now?

Yes, I believe updating the patch would be good. :-)


(In reply to comment #7)
> So it'd be good to go back to something like that.

This sounds like a good general improvement. Nevertheless, I'd vote for fixing this specific issue and creating a different issue for dealing with the improvement, as I suspect it may become somehow lengthy, in both time and code changes... :-|  Naturally, this comes from the assumption that the trunk should be as stable as possible for daily use, I may be wrong in assuming that. ;-)


Updating the platform to "All" as this was reproduced in Windows also (and, given that it is Batik-related, it should be reproducible in all supported platforms).
Comment 9 Daniel Westheide 2009-03-04 04:46:41 UTC
Created attachment 23327 [details]
Patch for the bug with correct code formatting.

I added a new version of the patch (attachment 23321 [details]). The only difference is in the code formatting, which was not quite correct in the original version.
Comment 10 Cameron McCormack 2009-03-18 23:24:19 UTC
I want to look at committing this patch.  Thomas, do you think it is large enough need a CLA to be submitted?
Comment 11 Thomas Deweese 2009-03-23 05:07:49 UTC
(In reply to comment #10)
> I want to look at committing this patch.  Thomas, do you think it is large
> enough need a CLA to be submitted?

  It's certainly on the borderline.  If it weren't for the fact that
the change is almost identical to the GenericElementNS code I'd probably
say yes.  Given the uncertainty I think it's best if we get a CLA.

  Daniel can you file a Contributors License Agreement?
     http://www.apache.org/licenses/
Comment 12 Daniel Westheide 2009-03-25 23:36:30 UTC
(In reply to comment #11)
 >   Daniel can you file a Contributors License Agreement?
>      http://www.apache.org/licenses/

Since I created this particular patch as part of my job, I need to clear this up with my employer. I'm afraid this might take a couple of days. Of course, if you consider this patch trivial enough that it's viable to forego a CLA, then please do it. Sorry for the delay. I was not aware that a CLA might be necessary for contributing this patch.
Comment 13 Helder Magalhães 2009-10-27 16:50:08 UTC
(In reply to comment #12)
> Since I created this particular patch as part of my job, I need to clear this
> up with my employer. I'm afraid this might take a couple of days.

A little more than a couple of days have passed: in fact, quite more than a couple of months! :-D

Daniel, please don't leave this unattended in your TODO stack forever: I already saw a couple of good patches rotting in the bug tracker due to lack of a CLA. :-|

Before reading the full Apache v2 license agreement, you may be interested in taking a quick look at a human-readable summary [1]. ;-)


[1] http://www.codeproject.com/info/Licenses.aspx#ctl00_MC_LR_ctl08_N
Comment 14 Daniel Westheide 2009-11-06 07:33:09 UTC
(In reply to comment #13)
 > Daniel, please don't leave this unattended in your TODO stack forever: I
> already saw a couple of good patches rotting in the bug tracker due to lack of
> a CLA. :-|

Helder, I am very sorry that this process takes such a long time, much longer than I myself had expected. In fact, I attended to this issue back in March already, but I had problems communicating this to those in charge of deciding about it.

However, I pushed this topic once more now and was told that I can expect feedback within a couple of weeks.

I hope you understand that I cannot sign the CLA until I have clearance from my employer.
Comment 15 Helder Magalhães 2009-11-07 02:09:46 UTC
(In reply to comment #14)
> Helder, I am very sorry that this process takes such a long time, much longer
> than I myself had expected. In fact, I attended to this issue back in March
> already, but I had problems communicating this to those in charge of deciding
> about it.

No problem, I have similar things happening on my side. Whenever it's required management intervention, things can take a while (and/or even get a bit complicated)... :-|


> However, I pushed this topic once more now and was told that I can expect
> feedback within a couple of weeks.

Great, thanks. :-)


> I hope you understand that I cannot sign the CLA until I have clearance from my
> employer.

Yes, of course that's reasonable: I was just trying to avoid (given the lack of an update) that this would be hanging forever. ;-)
Comment 16 Helder Magalhães 2010-08-22 11:50:14 UTC
(In reply to comment #15)
> > I hope you understand that I cannot sign the CLA until I have clearance from my
> > employer.
> 
> Yes, of course that's reasonable: I was just trying to avoid (given the lack of
> an update) that this would be hanging forever. ;-)

@Daniel:
Ping! Again trying to push this up in the stack, possibly taking advantage of employers returning from vacation with extra-energy to deal with these sort of things. ;-)
Comment 17 Helder Magalhães 2010-10-06 05:37:28 UTC
Setting this issue to block the 1.8 release (bug 50045). It's a relevant bug, there's a fix already available (and almost ready for integration) and SVG 1.2 documents are probably starting to rise as SVG 1.1 is getting solidly deployed everywhere.