Last Comment Bug 824324 - JavaScript strict warning: chrome://global/content/bindings/radio.xml, line 133: reference to undefined property val.value
: JavaScript strict warning: chrome://global/content/bindings/radio.xml, line 1...
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Account Manager (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 21.0
Assigned To: :aceman
:
Mentors:
Depends on:
Blocks: 826732
  Show dependency treegraph
 
Reported: 2012-12-23 00:44 PST by ISHIKAWA, Chiaki
Modified: 2013-02-04 10:43 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch to fix radio.xml to use val.getAttribute('value') (850 bytes, patch)
2012-12-23 00:44 PST, ISHIKAWA, Chiaki
no flags Details | Diff | Review
An excerpt from make mozmill run of local DEBUG BUILD of TB (104.83 KB, text/plain)
2012-12-23 16:05 PST, ISHIKAWA, Chiaki
no flags Details
patch (1.55 KB, patch)
2013-01-08 14:19 PST, :aceman
iann_bugzilla: review+
ishikawa: feedback+
Details | Diff | Review
patch v2 (2.10 KB, patch)
2013-01-28 13:16 PST, :aceman
iann_bugzilla: review+
Details | Diff | Review

Description ISHIKAWA, Chiaki 2012-12-23 00:44:44 PST
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'));
           }
Comment 1 ISHIKAWA, Chiaki 2012-12-23 02:04:43 PST
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
Comment 2 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-12-23 09:16:40 PST
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.
Comment 3 ISHIKAWA, Chiaki 2012-12-23 16:04:26 PST
(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.
Comment 4 ISHIKAWA, Chiaki 2012-12-23 16:05:11 PST
Created attachment 695372 [details]
An excerpt from make mozmill run of local DEBUG BUILD of TB
Comment 5 :aceman 2013-01-08 03:35:52 PST
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 .
Comment 6 ISHIKAWA, Chiaki 2013-01-08 05:12:43 PST
(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.
Comment 7 :aceman 2013-01-08 14:19:13 PST
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?
Comment 8 :aceman 2013-01-08 15:06:08 PST
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.
Comment 9 ISHIKAWA, Chiaki 2013-01-09 10:24:58 PST
(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
Comment 10 Ian Neal 2013-01-26 05:52:10 PST
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.
Comment 11 :aceman 2013-01-26 13:44:20 PST
I am not sure that is the same. Setting .value may not initialize .selectedItem.
Comment 12 :aceman 2013-01-26 13:48:46 PST
The change with the .querySelector() is actually not required to fix the bug.
Comment 13 neil@parkwaycc.co.uk 2013-01-26 15:52:26 PST
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;
Comment 14 :aceman 2013-01-28 13:16:46 PST
Created attachment 707264 [details] [diff] [review]
patch v2
Comment 15 Ryan VanderMeulen [:RyanVM] 2013-02-04 10:43:34 PST
https://hg.mozilla.org/comm-central/rev/35d92e1faf46

Note You need to log in before you can comment on or make changes to this bug.