JDK 8 Code Review Request for 7195301: XML Signature DOM implementation should not use instanceof to determine type of Node

[Sean Mullan]

On 9/7/12 11:45 PM, Xuelei Fan wrote:
> Looks fine to me.
> 
> It takes me to think whether it is safe to cast an object of
> ELEMENT_NODE type to org.w3c.dom.Element.
> 
> For example, in RetrievalMethodResolver.java
> --------------------------------------------------------
>  281    static Element getDocumentElement(Set<Node> set) {
>  282            Iterator<Node> it=set.iterator();
>  283            Element e=null;
>  284            while (it.hasNext()) {
>  285                    Node currentNode=it.next();
> -286                    if (currentNode instanceof Element) {
> +286                    if (currentNode != null &&
> currentNode.getNodeType() == Node.ELEMENT_NODE) {
>  287                            e=(Element)currentNode;
>  288                            break;
>  289                    }
>  290
>  291            }
> --------------------------------------------------------
> 
> It is safe because the element is come from the org.w3c.dom.Node. Using
> "instanceof" looks more straightforward.  Maybe you want to use the same
> style to check the node type.

Actually, that could still cause issues. The reason this bug was found was
because there is a DOM implementation that returns a Node that is an instanceof
Element and Document, but the node type is Document (this is an odd design, but
it is still technically compliant with the DOM API). This was causing the
Document to be incorrectly casted to an Element.

Thanks,
Sean

> 
> The same for CanonicalizerBase.java,
> 
> Anyway, it is just a very very minor concern. The fix is fine to me.
> 
> Xuelei
> 
> On 9/7/2012 10:39 PM, Sean Mullan wrote:
>> Hi Xuelei,
>>
>> Would you be able to review my fix for 7195301?
>>
>> webrev: http://cr.openjdk.java.net/~mullan/webrevs/7195301/webrev.00/
>>
>> No regression test (noreg-hard), requires complex setup, but I have attached a
>> test to the bug report that can be run manually.
>>
>> Thanks,
>> Sean
>>
>