Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

isXMLDoc() - Uncaught TypeError: Cannot read property 'namespaceURI' of undefined #4782

Closed
borzaka opened this issue Aug 29, 2020 · 11 comments · Fixed by #4814 or jquery/sizzle#474
Closed
Assignees
Milestone

Comments

@borzaka
Copy link

borzaka commented Aug 29, 2020

Description

isXMLDoc() throws an error if undefined given, starting from version 3.4.0

Test script:

jQuery.isXMLDoc(undefined)

jQuery 3.3.1 result:

false

jQuery 3.5.1 (currently the latest, but starting from 3.4.0) result:

Uncaught TypeError: Cannot read property 'namespaceURI' of undefined

Techinfo

Before the Sizzle update from 2.3.3 to 2.3.4 (see related commit below), isXML() function looked liked this:

isXML = Sizzle.isXML = function( elem ) {
	// documentElement is verified for cases where it doesn't yet exist
	// (such as loading iframes in IE - #4833)
	var documentElement = elem && (elem.ownerDocument || elem).documentElement;
	return documentElement ? documentElement.nodeName !== "HTML" : false;
};

This doesn't throw an error, when the elem is undefined, because it checks it.

In the update from 3.4.0, it looks like this:

isXML = Sizzle.isXML = function( elem ) {
	var namespace = elem.namespaceURI,
		docElem = ( elem.ownerDocument || elem ).documentElement;

	// Support: IE <=8
	// Assume HTML when documentElement doesn't yet exist, such as inside loading iframes
	// https://bugs.jquery.com/ticket/4833
	return !rhtml.test( namespace || docElem && docElem.nodeName || "HTML" );
};

This throws an error when the elem is undefined:
Uncaught TypeError: Cannot read property 'namespaceURI' of undefined

Please, if you can, bring back the previous functionality: undefined is not an XML node, so it should return a false boolean instead of a TypeError.

Thanks in advance!

Additional info

Related commit: d940bc0 (from line 562)
Related blog-post: https://blog.jquery.com/2019/04/10/jquery-3-4-0-released/

@dmethvin
Copy link
Member

Can you post a test case showing when this happens? Also can you use the latest jQuery 3 in your test case?

Be aware that version 2.x is no longer supported. If there is a bug to fix we'd only be fixing it in jQuery 3.x, or 4.x if it requires a breaking change.

@borzaka
Copy link
Author

borzaka commented Aug 30, 2020

I'am afraid if I present my use case, the attention will move away from my finding. Which is:
jQuery 3.3.1 returns a different outcome for the same function and argument then 3.5.1

jQuery 3.3.1 returns: false
jQuery 3.5.1 returns: Uncaught TypeError: Cannot read property 'namespaceURI' of undefined
For this line of code: jQuery.isXMLDoc(undefined)

jQuery 3.5.1 doesn't check the argument, 3.3.1 does. undefined is not an XML node, so in my interpretation it should return false, as version 3.3.1 does.

I'am talking about jQuery 3.x, not 2.x, only 2.x I mentioned is Sizzle update from 2.3.3 to 2.3.4, where this isXML() function is changed and causing this new TypeError since 3.4.0. Linked under Additional info in my original post.

@mgol
Copy link
Member

mgol commented Aug 30, 2020

As indicated in the signature of that method on its API page, only arguments of Element type are allowed as input to that method. Behavior for all other inputs is undefined as jQuery generally doesn't validate whether input is of a correct type.

If you want to make sure you only use allowed types, using TypeScript with jQuery typings is a good idea.

I'll let it open for discussion for now but generally we've been leaning towards not handling undefined inputs in APIs where it's not expected.

@mgol mgol added the Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. label Aug 30, 2020
@timmywil
Copy link
Member

We're deliberating whether we want to add back the falsey check.

@timmywil
Copy link
Member

I think we're willing to add back the falsey check, but we'd all feel better if there was a valid use case presented.

the attention will move away from my finding

Just the opposite btw. It will grant some validity to your finding.

@timmywil timmywil added Needs info and removed Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. labels Sep 21, 2020
@borzaka
Copy link
Author

borzaka commented Nov 6, 2020

Sorry for the late reply. Here is a part of that code working with ajax responses.

mainComplete = function(xhr, status) {
    var result
    var resultText

    if ($.isXMLDoc(xhr.responseXML) && xhr.responseXML.documentElement.nodeName === 'result') {
        result = processCoreResponse(xhr.responseXML)
    }

    resultText = result ? (result.content[''] || '') : xhr.responseText

    ...
}

The $.isXMLDoc(xhr.responseXML) is failing because of the lack of the falsey check as before.

Could you reopen this issue please?

@mgol
Copy link
Member

mgol commented Nov 6, 2020

Thanks for the use case, reopening.

@mgol mgol reopened this Nov 6, 2020
@mgol mgol added Core Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. and removed Needs info labels Nov 6, 2020
@timmywil timmywil removed the Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. label Nov 9, 2020
@timmywil timmywil added this to the 3.6.0 milestone Nov 9, 2020
@mgol mgol self-assigned this Dec 1, 2020
mgol added a commit to mgol/jquery that referenced this issue Dec 1, 2020
mgol added a commit to mgol/sizzle that referenced this issue Dec 1, 2020
mgol added a commit to mgol/sizzle that referenced this issue Dec 1, 2020
@mgol
Copy link
Member

mgol commented Dec 1, 2020

PR to jQuery (master branch, for jQuery 4.0): #4814
PR to Sizzle (for jQuery 3.6.0): jquery/sizzle#474

mgol added a commit to mgol/jquery that referenced this issue Dec 1, 2020
mgol added a commit to mgol/jquery that referenced this issue Dec 1, 2020
mgol added a commit to mgol/sizzle that referenced this issue Dec 1, 2020
@mgol mgol reopened this Dec 7, 2020
@mgol mgol closed this as completed in #4814 Dec 7, 2020
mgol added a commit that referenced this issue Dec 7, 2020
@mgol mgol reopened this Dec 7, 2020
@mgol
Copy link
Member

mgol commented Dec 7, 2020

Landed in jquery/sizzle#474 & #4814 but we still need to update Sizzle on 3.x-stable.

@mgol
Copy link
Member

mgol commented Feb 15, 2021

Reopening as it awaits a Sizzle update.

@mgol mgol reopened this Feb 15, 2021
mgol added a commit to mgol/jquery that referenced this issue Feb 17, 2021
timmywil pushed a commit that referenced this issue Feb 17, 2021
@mgol
Copy link
Member

mgol commented Feb 18, 2021

Fixed on 3.x-stable (jQuery 3.x) by #4846.
Previously, it was only fixed on main (jQuery 4.0) by #4814.

Closing.

@mgol mgol closed this as completed Feb 18, 2021
mgol added a commit to mgol/jquery that referenced this issue Sep 8, 2022
mgol added a commit to mgol/jquery that referenced this issue Sep 12, 2022
mgol added a commit to mgol/jquery that referenced this issue Sep 19, 2022
mgol added a commit to mgol/jquery that referenced this issue Sep 21, 2022
mgol added a commit to mgol/jquery that referenced this issue Oct 3, 2022
mgol added a commit to mgol/jquery that referenced this issue Oct 4, 2022
mgol added a commit to mgol/jquery that referenced this issue Nov 17, 2022
mgol added a commit to mgol/jquery that referenced this issue Dec 1, 2022
mgol added a commit to mgol/jquery that referenced this issue Dec 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment