[JENKINS-53182] Uncaught TypeError: Cannot read property 'firstChild' of undefined - updateListBox #3645

sai-tirunagiri · 2018-09-24T12:35:11Z

See JENKINS-53182.

Proposed changelog entries

Fix - Uncaught TypeError: Cannot read property 'firstChild' of undefined - updateListBox

Submitter checklist

JIRA issue is well described
Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
* Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
[n/a] Appropriate autotests or explanation to why this change has no tests
[n/a] For dependency updates: links to external changelogs and, if possible, full diffs

oleg-nenashev

Would be nice to understand what causes the issue. Could it be a custom Javascript from a theme?

No strong objection tho

oleg-nenashev · 2018-09-26T11:06:48Z

core/src/main/resources/lib/form/select/select.js

-        status.innerHTML = "";
+    var followingTR = findFollowingTR(listBox, "validation-error-area");
+    if(followingTR!=undefined){
+    	var status = findFollowingTR(listBox, "validation-error-area").firstChild.nextSibling;


Use the existing field instead of repeating findFollowingTR () ?

This has been taken care of by having an exception handler in place Oleg. Ideally, it reduces the overhead of using findFollowingTR () twice.

oleg-nenashev · 2018-09-26T11:07:42Z

war/src/main/webapp/scripts/hudson-behavior.js

@@ -304,15 +304,17 @@ function findAncestorClass(e, cssClass) {
 function findFollowingTR(input, className) {
    // identify the parent TR
    var tr = input;
-    while (tr.tagName != "TR")
+    if(tr.tagName!=undefined){
+      while (tr.tagName != "TR")
        tr = tr.parentNode;


can the parentNode be undefined as well? I believe no, but just in case

Make sense. So, I have added validation even on parentNode.

sai-tirunagiri · 2018-09-27T08:56:12Z

Question : Cause for this issue-

Trying to use the method updateListBox() function in jelly for an internal plugin development. This function call caused the issue of uncaught error.

oleg-nenashev

Would be better to revert formatting changes.

Regarding the code itself, it would be great to get feedback from @jenkinsci/code-reviewers .

sai-tirunagiri · 2018-09-28T07:16:51Z

Looking forward to the feedback.

"revert formatting changes" Can you help me understand more on this.? Thanks

oleg-nenashev · 2018-10-06T14:14:57Z

@sai-tirunagiri Many changes in the diff are just whitespace ones. E.g. compare this line with https://github.com/jenkinsci/jenkins/pull/3645/files?w=1 . The one without formatting contains only 10 added lines while the formatted diff displays 20

daniel-beck · 2018-10-08T08:36:35Z

@oleg-nenashev

The one without formatting contains only 10 added lines while the formatted diff displays 20

This is due to adding a level of indentation. Not doing that just hurts future legibility unnecessarily.

daniel-beck · 2018-10-08T08:37:27Z

OTOH the file currently has indentation of 4 spaces not 2, so this PR adds an inconsistency.

sai-tirunagiri · 2018-10-08T13:10:23Z

@oleg-nenashev @daniel-beck , Thank you for the feedback on this. I have corrected the formatting issues. Please check and let me know.

sai-tirunagiri · 2018-10-08T13:11:26Z

@sai-tirunagiri Many changes in the diff are just whitespace ones. E.g. compare this line with https://github.com/jenkinsci/jenkins/pull/3645/files?w=1 . The one without formatting contains only 10 added lines while the formatted diff displays 20

Thank you for the details. This helped me to understand.

daniel-beck · 2018-10-09T20:26:36Z

4fdf212 is bad. Please undo this. Lines should have their natural indentation, and if newly placed in a block, should be indented.

sai-tirunagiri · 2018-10-10T07:00:09Z

@daniel-beck , does this change look good to you.

daniel-beck · 2018-10-13T17:13:25Z

war/src/main/webapp/scripts/hudson-behavior.js

    while (tr.tagName != "TR")
        tr = tr.parentNode;

    // then next TR that matches the CSS
    do {
        tr = $(tr).next();
    } while (tr != null && (tr.tagName != "TR" || !Element.hasClassName(tr,className)));
-
    return tr;


I don't understand why this line and preceding ones aren't indented.

daniel-beck · 2018-10-13T17:14:03Z

war/src/main/webapp/scripts/hudson-behavior.js

@@ -304,15 +304,17 @@ function findAncestorClass(e, cssClass) {
 function findFollowingTR(input, className) {
    // identify the parent TR
    var tr = input;
+    if(tr.tagName!=undefined && tr.parentNode!=undefined){


Lack of spaces around parentheses and operators is inconsistent with the rest of the file (such as the one immediately below).

This line of code has been removed. Since the exception handling is being done by the caller itself.

daniel-beck · 2018-10-13T17:14:08Z

core/src/main/resources/lib/form/select/select.js

+        status = findFollowingTR(listBox, "validation-error-area").firstChild.nextSibling;
+        if (status.firstChild && status.firstChild.getAttribute('data-select-ajax-error'))
+            status.innerHTML = "";
+    }catch(e){


Missing spaces

Done with the changes as per the comment.

daniel-beck · 2018-10-13T17:14:11Z

core/src/main/resources/lib/form/select/select.js

-    if (status.firstChild && status.firstChild.getAttribute('data-select-ajax-error')) {
-        status.innerHTML = "";
+    var status;
+    try{


Missing spaces

daniel-beck · 2018-10-13T17:14:49Z

Please try to adapt to the existing code style when editing a file (even if the entire code base might be a bit inconsistent overall).

sai-tirunagiri · 2018-10-15T06:43:28Z

@daniel-beck , Apologies for not following the expected standards in the code. But did the changes as requested. Please review and let me know.

I removed the undefined check and the operator from hudson-behaviour.js since, exception handling is being done on select.js itself (exception handling is done on the calling function)

sai-tirunagiri · 2018-10-23T06:22:32Z

@oleg-nenashev , @daniel-beck any updates on this pull request. Thanks.

oleg-nenashev · 2018-12-14T18:00:02Z

Sorry, I totally missed the pings due to the conferences

oleg-nenashev · 2018-12-14T18:07:45Z

No opinion, now it just suppresses exceptions.
CC @kzantow @recena and other @jenkinsci/code-reviewers with JS expertise

sai-tirunagiri · 2019-01-20T16:39:36Z

@oleg-nenashev @jenkinsci/code-reviewers.. any update on this pull request would be appreciated. Thanks.

sai-tirunagiri · 2019-01-28T15:05:56Z

@daniel-beck @oleg-nenashev @kzantow - Please provide an update on this pull request. I am tired of dealing with this exception.

oleg-nenashev

I still do not have any opinion about that. It still looks like suppressing a real defect in a custom plugin to me && I do not have enough Javascript expertise, so I am not ready to approve it

oleg-nenashev · 2019-01-28T15:10:12Z

core/src/main/resources/lib/form/select/select.js

@@ -98,4 +103,4 @@ Behaviour.specify("SELECT.select", 'select', 1000, function(e) {
                }
            });
        });
-});
+});


Suggested change

});

});

Is there linting in place to guarantee newline at end of file?

👍 @oleg-nenashev

oleg-nenashev · 2019-01-28T15:10:20Z

war/src/main/webapp/scripts/hudson-behavior.js

@@ -3053,4 +3051,4 @@ var notificationBar = {
        if (!options.sticky)
            this.token = window.setTimeout(function(){self.hide();},this.DELAY);
    }
-};
+};


Suggested change

};

};

codebryo · 2019-01-28T15:18:06Z

core/src/main/resources/lib/form/select/select.js

+            status.innerHTML = "";
+    } 
+    catch(e) {
+        console.log(e);


If it's a critical error it should probably be printed out with a console.error or console.warn

kzantow · 2019-01-28T15:18:34Z

core/src/main/resources/lib/form/select/select.js

+            status.innerHTML = "";
+    } 
+    catch(e) {
+        console.log(e);


this doesn't really solve the problem, does it? just logs instead of throwing?

I suggest:

find out what scenario causes this problem
and

either extract out checks for .firstChild and .nextSibling or change a selector such that it fixes the root cause of the problem.

Jenkins console error:-

jelly config file

So, this error is caused by updateListBox function.. Any suggestions on fixing this would be great.

Fyi... the same works totally fine on jenkins verion 2.7.3

oleg-nenashev · 2019-01-31T00:17:30Z

@jsoref has proposed another fix in #3645 . I believe that fix solves this issue as well

oleg-nenashev · 2019-01-31T00:18:33Z

I might be wrong tho

sai-tirunagiri · 2019-02-04T06:33:29Z

@oleg-nenashev - Thanks for the update. I will wait for it then.

jglick · 2019-02-07T20:47:36Z

@jsoref has proposed another fix in #3645

@oleg-nenashev huh? That is this PR. Which do you mean?

jsoref · 2019-02-07T21:49:58Z

@oleg-nenashev: did you mean #3869?

jsoref · 2019-02-07T21:54:11Z

I have too many things in too many states to be sure. I also have one thing which will significantly rework how hudson-behavior and these jelly files handle things (#3859).

I'm not sure which change is likely to be relevant. I also have a couple of changes which adjust handling of empty children. But yes, I do think that checking again after a release would be a good idea.

oleg-nenashev · 2019-02-08T11:01:41Z

@sai-tirunagiri Would you be able to test your plugin with https://repo.jenkins-ci.org/incrementals/org/jenkins-ci/main/jenkins-war/2.164-rc27955.2f8b557a3bac/ to confirm whether the issue is fixed by the already integrated fixes?

jsoref · 2019-02-08T19:23:19Z

So, I still I hit this.

In my case, the select is in
#main-panel > form > div > div:nth-child(133) > div.setting-main > div > div:

<div name="libraries" class="repeated-chunk to-be-removed">
  <div class="dd-handle">Library</div>
...
<!-- #main-panel > form > div > div:nth-child(133) > div.setting-main > div > div > div:nth-child(2) > div:nth-child(39) > div.setting-main > div > div > select -->
<select fillurl="/descriptorByName/hudson.plugins.mercurial.MercurialSCMSource/fillCredentialsIdItems" filldependson="source" name="_.credentialsId" class="setting-input  select  credentials-select" value="">
</select>

The select is for:

<div class="setting-name">Credentials
  <span class="setting-help">
    <a helpurl="/descriptor/hudson.plugins.mercurial.MercurialSCMSource/help/credentialsId" href="#" class="help-button"><img
src="/static/34c69582/images/16x16/help.png" alt="Help for feature: Credentials" style="width: 16px; height: 16px; " class="icon-help icon-sm"
    ></a>
  </span>
</div>

In this case, the problem is that the select is not generated by jenkinsci/jenkins jelly, it's from: https://github.com/jenkinsci/mercurial-plugin/blob/288e4dd0a629c7071b09305d4c238ff117fd7187/src/main/resources/hudson/plugins/mercurial/MercurialSCM/config.jelly#L20

The right fix is thus not to hack the JS here in jenkins the way that's being suggested. It would be to instead to add an extra attribute to the select, check for that, and then only try to run this code if that attribute is present.

👎

More precisely, it's this widget
https://github.com/jenkinsci/credentials-plugin/blob/07728f1a2f16046f27a3e84585356d4b10359c6d/src/main/resources/lib/credentials/select.jelly

I'm still looking through, but it really feels like something is responsible for the field not having the item ...

sai-tirunagiri · 2019-02-09T12:50:38Z

@oleg-nenashev : Its still the same.

Uncaught TypeError: Cannot read property 'firstChild' of undefined
    at updateListBox (select.js:8)
    at select.js:79
    at h (hudson-behavior.js:1304)
    at refillOnChange (hudson-behavior.js:1319)
    at select.js:77
    at Array.forEach (<anonymous>)
    at behavior.js:111
    at Array.forEach (<anonymous>)
    at behavior.js:107
    at Array.forEach (<anonymous>)

sai-tirunagiri · 2019-02-09T14:23:11Z

@jsoref : I did follow your inputs and tried organizing the config.jelly a bit. after doing these changes in jelly..

<f:entry field="configName" title="Choose Configuration Name">
 <f:select onchange="updateListBox('team_info','descriptorByName/org.jenkinsci.plugins.ctc.TestAutomationBuilder/teamInfo?value='+encode(this.value))"/>
</f:entry>

<f:entry field="teamInfo" title="Choose Your Team">
    <f:select name="team_info" id="team_info"/>
</f:entry>

ended up getting the below error.

Uncaught TypeError: Cannot read property 'tagName' of undefined
    at findFollowingTR (hudson-behavior.js:307)
    at updateListBox (select.js:8)
    at HTMLSelectElement.onchange (configure:1)

any clue.?

jsoref · 2019-02-11T18:50:10Z

@sai-tirunagiri: sorry, I'm still looking into this.

jsoref · 2019-02-14T16:14:11Z

FWIW, I've put in 13cd57d (into my PR) which I think will limit the fallout from errors like this. It isn't a fix for the underlying problem, but I think it's a helpful step for resiliency.

sai-tirunagiri · 2019-02-17T11:05:03Z

Thank you @jsoref .. So, this will be available to me in the next release of Jenkins is it?

jsoref · 2019-02-17T14:46:33Z

No. I'm still tinkering with it. (I'm going to include exception logging, otherwise debugging is painful.)

sai-tirunagiri · 2019-02-19T05:29:20Z

Agreed. Please do keep this thread updated as its an important patch for us. @jsoref

sai-tirunagiri · 2019-02-27T06:46:14Z

@jsoref : we do have the same issue with different users.

daniel-beck · 2019-04-10T08:46:48Z

@oleg-nenashev Could you clarify why you put this on-hold? To wait for the response whether it's already fixed that was provided within a day?

jglick · 2019-04-19T12:48:34Z

This kind of error message is a longstanding problem in Jenkins. There is not a single root cause, but rather a variety of mistakes such as malformed plugin Jelly files (one example: forgetting to wrap some elements in a table row), unexpected null values or Java exceptions, etc. which are not being properly detected and recovered from gracefully. Merely printing a JavaScript error is unlikely to provide meaningful diagnostics. If the specific instance of the problem motivating this particular issue is reproducible, then you need to track down the root cause and add proper error handling at the source. Additionally, I would expect fixes like this to be accompanied by a regression test, probably using HtmlUnit.

alecharp · 2019-08-01T10:30:50Z

@sai-tirunagiri Would you have time to come back on this PR?

varyvol · 2019-08-29T09:22:19Z

In the current state, I think it doesn't really solve the issue but hides it somehow. We should work in fixing the real problem.

varyvol · 2019-11-14T10:50:02Z

We are proposing this closure given there's been no follow up for some time.

sai-tirunagiri · 2019-11-15T07:54:00Z

Yes, we can close this. I did a workaround using ajax in jelly which seems to have sorted this problem. But, we still need to find out the exact root cause of this issue.

Thanks.

Sai Prasad TN added 2 commits September 24, 2018 10:14

Fix - Uncaugth TypeError firstChild of undefined

8c522d2

Removing console log from js

a3602e8

oleg-nenashev reviewed Sep 26, 2018

View reviewed changes

adding exception handler and validating parentNode

9af084c

oleg-nenashev reviewed Sep 27, 2018

View reviewed changes

Sai Prasad TN added 2 commits October 8, 2018 13:02

reverting line indentations

6548701

reverting line indentations on select library

4fdf212

correcting line indentations

b86fcc4

daniel-beck requested changes Oct 13, 2018

View reviewed changes

formatting and removing the comparison operator in hudson-behaviour

dc43634

oleg-nenashev requested review from oleg-nenashev and daniel-beck December 14, 2018 18:06

oleg-nenashev reviewed Jan 28, 2019

View reviewed changes

oleg-nenashev changed the title ~~Jenkins 53182 Uncaught TypeError: Cannot read property 'firstChild' of undefined - updateListBox~~ [JENKINS-53182] Uncaught TypeError: Cannot read property 'firstChild' of undefined - updateListBox Jan 28, 2019

codebryo reviewed Jan 28, 2019

View reviewed changes

kzantow reviewed Jan 28, 2019

View reviewed changes

oleg-nenashev added the proposed-for-close There is no consensus about feasibility of this PR, and maintainers do not see a way forward for it label Jan 31, 2019

oleg-nenashev added on-hold This pull request depends on another event/release, and it cannot be merged right now and removed proposed-for-close There is no consensus about feasibility of this PR, and maintainers do not see a way forward for it labels Feb 8, 2019

oleg-nenashev added the stalled The PR is reasonable and might be merged, but it is no longer active. It can be taken over by other label Aug 29, 2019

varyvol added the proposed-for-close There is no consensus about feasibility of this PR, and maintainers do not see a way forward for it label Nov 14, 2019

sai-tirunagiri closed this Nov 15, 2019

[JENKINS-53182] Uncaught TypeError: Cannot read property 'firstChild' of undefined - updateListBox #3645

[JENKINS-53182] Uncaught TypeError: Cannot read property 'firstChild' of undefined - updateListBox #3645

Conversation

sai-tirunagiri commented Sep 24, 2018

Proposed changelog entries

Submitter checklist

oleg-nenashev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sai-tirunagiri commented Sep 27, 2018

oleg-nenashev left a comment

Choose a reason for hiding this comment

sai-tirunagiri commented Sep 28, 2018

oleg-nenashev commented Oct 6, 2018

daniel-beck commented Oct 8, 2018

daniel-beck commented Oct 8, 2018

sai-tirunagiri commented Oct 8, 2018

sai-tirunagiri commented Oct 8, 2018

daniel-beck commented Oct 9, 2018

sai-tirunagiri commented Oct 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-beck commented Oct 13, 2018

sai-tirunagiri commented Oct 15, 2018

sai-tirunagiri commented Oct 23, 2018

oleg-nenashev commented Dec 14, 2018

oleg-nenashev commented Dec 14, 2018

sai-tirunagiri commented Jan 20, 2019

sai-tirunagiri commented Jan 28, 2019

oleg-nenashev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-nenashev commented Jan 31, 2019

oleg-nenashev commented Jan 31, 2019

sai-tirunagiri commented Feb 4, 2019

jglick commented Feb 7, 2019

jsoref commented Feb 7, 2019

jsoref commented Feb 7, 2019

oleg-nenashev commented Feb 8, 2019

jsoref commented Feb 8, 2019 • edited Loading

sai-tirunagiri commented Feb 9, 2019 • edited Loading

sai-tirunagiri commented Feb 9, 2019 • edited Loading

jsoref commented Feb 11, 2019

jsoref commented Feb 14, 2019

sai-tirunagiri commented Feb 17, 2019

jsoref commented Feb 17, 2019

sai-tirunagiri commented Feb 19, 2019

sai-tirunagiri commented Feb 27, 2019

daniel-beck commented Apr 10, 2019

jglick commented Apr 19, 2019

alecharp commented Aug 1, 2019

varyvol commented Aug 29, 2019

varyvol commented Nov 14, 2019

sai-tirunagiri commented Nov 15, 2019

jsoref commented Feb 8, 2019 •

edited

Loading

sai-tirunagiri commented Feb 9, 2019 •

edited

Loading

sai-tirunagiri commented Feb 9, 2019 •

edited

Loading