JavaScript strict warning: chrome://global/content/bindings/radio.xml, line 133: reference to undefined property val.value

RESOLVED FIXED in Thunderbird 21.0

Status

MailNews Core
Account Manager
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: ISHIKAWA, Chiaki, Assigned: aceman)

Tracking

Trunk
Thunderbird 21.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
Created attachment 695288 [details] [diff] [review]
patch to fix radio.xml to use val.getAttribute('value')

During a test run of "make mozmill" of local DEBUG BUILD of
thunderbird (comm-central), I noticed the following warning:
thunderbird version is
$ hg identify
99fd4744910d+ tip

JavaScript strict warning: chrome://global/content/bindings/radio.xml, line 133: reference to undefined property val.value

radio.xml is comm-central's  mozilla/toolkit/content/widgets/radio.xml

Comparing the use of 'value' in other xml files under the same directory,
I think this is fixed by the attached patch.
I show the main change below:
             val.setAttribute("selected", "true");
-            this.setAttribute("value", val.value);
+            this.setAttribute("value", val.getAttribute('value'));
           }
(Reporter)

Comment 1

4 years ago
Comment on attachment 695288 [details] [diff] [review]
patch to fix radio.xml to use val.getAttribute('value')

I re-ran "make mozmill" with DEBUG BUILD of TB and no longer see the warning now.
So I think the patch is OK.

I put gavin's e-mail address since the
the last patch to this file was reviewed by him, it seems from what I learned about  the file using
http://hg.mozilla.org/mozilla-central/annotate/7b74f4ee76c9/toolkit/content/widgets/radio.xml
Attachment #695288 - Flags: review?(gavin.sharp)
Component: Widget → XUL Widgets
Product: Core → Toolkit
Comment on attachment 695288 [details] [diff] [review]
patch to fix radio.xml to use val.getAttribute('value')

"val" is supposed to be a "radio" element [1], whose binding inherits a "value" property from "control-item" [2].

[1] http://hg.mozilla.org/mozilla-central/annotate/d348dbf1dab4/toolkit/content/widgets/radio.xml#l435
[2] http://hg.mozilla.org/mozilla-central/annotate/d348dbf1dab4/toolkit/content/widgets/general.xml#l60

So it seems likely that some code somewhere is trying to set the radiogroup's selectedItem to a non-radio element (or a radio element whose binding is not attached?). I think we need to track down why this is happening exactly, before deciding on the fix. Using the attribute (this patch) is probably reasonable either way, but there may also be another bug in the consumer that this is masking.
Attachment #695288 - Attachment is patch: true
Attachment #695288 - Flags: review?(gavin.sharp)
(Reporter)

Comment 3

4 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #2)
> Comment on attachment 695288 [details] [diff] [review]
> patch to fix radio.xml to use val.getAttribute('value')
> 
> "val" is supposed to be a "radio" element [1], whose binding inherits a
> "value" property from "control-item" [2].
> 
 ...
> 
> So it seems likely that some code somewhere is trying to set the
> radiogroup's selectedItem to a non-radio element (or a radio element whose
> binding is not attached?). I think we need to track down why this is
> happening exactly, before deciding on the fix. Using the attribute (this
> patch) is probably reasonable either way, but there may also be another bug
> in the consumer that this is masking.

Thank you for the feedback. I am uploading an excerpt from a log of
make mozmill run.
This may help to locate the mentioned confusion of data types, then.
(Reporter)

Comment 4

4 years ago
Created attachment 695372 [details]
An excerpt from make mozmill run of local DEBUG BUILD of TB
(Assignee)

Comment 5

4 years ago
Also, .value and .getAttribute('value') are not always equivalent (at least for textbox elements).

I see this warning in the account manager (when manually running a debug build) and plan to find out which element is causing it.
I can try to take this bug if you don't mind, ISHIKAWA, chiaki .
(Reporter)

Comment 6

4 years ago
(In reply to :aceman from comment #5)
> Also, .value and .getAttribute('value') are not always equivalent (at least
> for textbox elements).
> 
> I see this warning in the account manager (when manually running a debug
> build) and plan to find out which element is causing it.
> I can try to take this bug if you don't mind, ISHIKAWA, chiaki .

Please take it then! I put your address in the assignee field.
Assignee: nobody → acelists
(Assignee)

Comment 7

4 years ago
Created attachment 699433 [details] [diff] [review]
patch

Ok, I have found the problem in the account manager. If there is no pref value for a radiogroup stored, we try to select the first child of it. But that sometimes is not a <radio> element. E.g. I got it on an IMAP account where the radiogroup(id="imap.deleteModel").firstChild was a <grid> element. So I change it to .getItemAtIndex(0) to let the radiogroup return the first item that is a radio. Seems to work. I do the same for menulist as that does the same unsafe assumption (that .firstChild.firstChild is a menuitem). Ishikawa, can you please run the tests again with this patch and see if you still get those warnings?
Attachment #695288 - Attachment is obsolete: true
Attachment #699433 - Flags: review?(iann_bugzilla)
Attachment #699433 - Flags: feedback?(ishikawa)
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
Component: XUL Widgets → Account Manager
Product: Toolkit → MailNews Core
Version: unspecified → Trunk
(Assignee)

Comment 8

4 years ago
I just noticed that your "excerpt from make mozmill run of local DEBUG BUILD of TB" shows the warning in one of the account manager tests, working on the Server settings pane. That is where I saw the problem at real usage and fixed it. So the patch should really help.
(Reporter)

Comment 9

4 years ago
(In reply to :aceman from comment #7)
> Created attachment 699433 [details] [diff] [review]
> patch

> Ishikawa, can you please run the tests again with this patch and see if you
> still get those warnings?

Good news. No more warning about val.value!
(sorry, I forgot how to clear the startup cache and so
changing .js file alone was not enough and had to re-run the build [using ccache certainly helps, but it took a few hours before the test was done.]

I hope this patch makes it to the official distribution soon.

TIA

TIA
Blocks: 826732
(Reporter)

Updated

4 years ago
Attachment #699433 - Flags: feedback?(ishikawa) → feedback+

Comment 10

4 years ago
Comment on attachment 699433 [details] [diff] [review]
patch

We should be able to rely on the xml, so this should be sufficient:
  else if (type == "radiogroup" || type =="menulist") {
    if (value == undefined)
      value = formElement.getItemAtIndex(0).value;
 
    formElement.value = value;

Might be worth double checking with Neil though.
Attachment #699433 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 11

4 years ago
I am not sure that is the same. Setting .value may not initialize .selectedItem.
Flags: needinfo?(neil)
(Assignee)

Comment 12

4 years ago
The change with the .querySelector() is actually not required to fix the bug.
Well, the XBL for both radio and menulist constructor tries to do this:
1. If an item has the selected attribute, it becomes the selected item
2. Otherwise, if the control has a value, the item with that value becomes selected
3. Otherwise, the selected index becomes zero

I don't think case 1 applies here, but it looks as if we could do
if (value == undefined)
  formElement.selectedIndex = 0;
else
  formElement.value = value;
Flags: needinfo?(neil)
(Assignee)

Comment 14

4 years ago
Created attachment 707264 [details] [diff] [review]
patch v2
Attachment #699433 - Attachment is obsolete: true
Attachment #707264 - Flags: review?(iann_bugzilla)

Updated

4 years ago
Attachment #707264 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
OS: Linux → All
Hardware: x86 → All
https://hg.mozilla.org/comm-central/rev/35d92e1faf46
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 21.0
You need to log in before you can comment on or make changes to this bug.