897052 - "System JS : ERROR resource://gre/modules/LoginManagerContent.jsm:169 \n TypeError: form is null" on green debug test runs

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[Ed Morley [:emorley]]

A soon to be deployed TBPL parser improvement has found a whole bunch of log spam in green Ubuntu debug mochitest runs.

a) Should these be making the test run fail?
b) We should either null check or fix the root cause if this is unexpected, so we don't spam the annotated failure summary once this TBPL patchset is rolled out.

eg:

Ubuntu VM 12.04 x64 mozilla-central debug test mochitest-1 on 2013-07-23 03:32:09 PDT for push fb4bf993a58a

slave: tst-linux64-ec2-012

https://tbpl-dev.allizom.org/php/getParsedLog.php?id=25603893&tree=Mozilla-Central#error1

{
03:52:07     INFO -  105751 INFO TEST-PASS | /tests/content/html/content/test/forms/test_button_attributes_reflection.html | IDL attribute 'value' should return the value it has been set to.
03:52:07     INFO -  105752 INFO TEST-PASS | /tests/content/html/content/test/forms/test_button_attributes_reflection.html | Content attribute 'value'should return the value it has been set to.
03:52:07     INFO -  105753 INFO TEST-PASS | /tests/content/html/content/test/forms/test_button_attributes_reflection.html | IDL attribute 'value' should return the value it has been set to.
03:52:07     INFO -  105754 INFO TEST-PASS | /tests/content/html/content/test/forms/test_button_attributes_reflection.html | Content attribute 'value' should return the value it has been set to.
03:52:07     INFO -  105755 INFO TEST-PASS | /tests/content/html/content/test/forms/test_button_attributes_reflection.html | When not set, the content attribute should be null.
03:52:07     INFO -  105756 INFO TEST-PASS | /tests/content/html/content/test/forms/test_button_attributes_reflection.html | When not set, the IDL attribute should return the empty string
03:52:07     INFO -  105757 INFO TEST-END | /tests/content/html/content/test/forms/test_button_attributes_reflection.html | finished in 800ms
03:52:07     INFO -  ++DOMWINDOW == 31 (0x60103f8) [serial = 2641] [outer = 0x3293508]
03:52:07     INFO -  System JS : ERROR (null):0
03:52:07     INFO -                       uncaught exception: 2147746065
03:52:07     INFO -  105758 INFO TEST-START | /tests/content/html/content/test/forms/test_change_event.html
03:52:07     INFO -  ++DOMWINDOW == 32 (0x64e58a8) [serial = 2642] [outer = 0x3293508]
03:52:07     INFO -  [Parent 2377] WARNING: zero axis length: file ../../../../../content/svg/content/src/nsSVGLength2.cpp, line 154
03:52:07     INFO -  [Parent 2377] WARNING: zero axis length: file ../../../../../content/svg/content/src/nsSVGLength2.cpp, line 154
03:52:07     INFO -  105759 INFO TEST-PASS | /tests/content/html/content/test/forms/test_change_event.html | Change event shouldn't be dispatched on text input element
03:52:07     INFO -  System JS : ERROR resource://gre/modules/LoginManagerContent.jsm:169
03:52:07     INFO -                       TypeError: form is null
03:52:07     INFO -  105760 INFO TEST-PASS | /tests/content/html/content/test/forms/test_change_event.html | text input element should have dispatched change event.
03:52:07     INFO -  105761 INFO TEST-PASS | /tests/content/html/content/test/forms/test_change_event.html | Change event shouldn't be dispatched on email input element
03:52:07     INFO -  System JS : ERROR resource://gre/modules/LoginManagerContent.jsm:169
03:52:07     INFO -                       TypeError: form is null
03:52:07     INFO -  105762 INFO TEST-PASS | /tests/content/html/content/test/forms/test_change_event.html | email input element should have dispatched change event.
03:52:07     INFO -  105763 INFO TEST-PASS | /tests/content/html/content/test/forms/test_change_event.html | Change event shouldn't be dispatched on search input element
03:52:07     INFO -  105764 INFO TEST-PASS | /tests/content/html/content/test/forms/test_change_event.html | search input element should have dispatched change event.
03:52:07     INFO -  105765 INFO TEST-PASS | /tests/content/html/content/test/forms/test_change_event.html | Change event shouldn't be dispatched on telephone input element
03:52:07     INFO -  System JS : ERROR resource://gre/modules/LoginManagerContent.jsm:169
03:52:07     INFO -                       TypeError: form is null
03:52:07     INFO -  105766 INFO TEST-PASS | /tests/content/html/content/test/forms/test_change_event.html | telephone input element should have dispatched change event.
03:52:07     INFO -  105767 INFO TEST-PASS | /tests/content/html/content/test/forms/test_change_event.html | Change event shouldn't be dispatched on url input element
03:52:07     INFO -  System JS : ERROR resource://gre/modules/LoginManagerContent.jsm:169
03:52:07     INFO -                       TypeError: form is null
03:52:07     INFO -  105768 INFO TEST-PASS | /tests/content/html/content/test/forms/test_change_event.html | url input element should have dispatched change event.
03:52:07     INFO -  105769 INFO TEST-PASS | /tests/content/html/content/test/forms/test_change_event.html | Change event shouldn't be dispatched on password input element
03:52:07     INFO -  System JS : ERROR (null):0
03:52:07     INFO -                       uncaught exception: 2147746065
}

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/LoginManagerContent.jsm#169

157     /*
158      * _getPasswordFields
159      *
160      * Returns an array of password field elements for the specified form.
161      * If no pw fields are found, or if more than 3 are found, then null
162      * is returned.
163      *
164      * skipEmptyFields can be set to ignore password fields with no value.
165      */
166     _getPasswordFields : function (form, skipEmptyFields) {
167         // Locate the password fields in the form.
168         var pwFields = [];
169         for (var i = 0; i < form.elements.length; i++) {
170             var element = form.elements[i];
171             if (!(element instanceof Ci.nsIDOMHTMLInputElement) ||
172                 element.type != "password")
173                 continue;
174 
175             if (skipEmptyFields && !element.value)
176                 continue;
177 
178             pwFields[pwFields.length] = {
179                                             index   : i,
180                                             element : element
181                                         };
182         }

Comment 1

[Justin Dolske [:Dolske]]

Attachment: Add null-check

I think this will fix it... The test is removing the <input> from the DOM (and it was never in a <form> to begin with), both of which result in input.form being null. It's harmless, but bug 839961 probably regressed this (for the second case; input-outside-a-form) because now we listen for all DOMAutoComplete events on the page, instead of just precisely for an input we are interested in.

Ed: is this trivial for you to apply and verify?

Comment 2

[Ryan VanderMeulen [:RyanVM]]

I would think you could verify that this works with a regular Try push. The log spam happens independently of the recent TBPL parsing changes that happen to notice it.

Comment 3

[Matthew N. [:MattN]]

Comment on attachment 780137 [details] [diff] [review]
Add null-check

r=me
Woo-hoo! First review as a peer!

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Pushed to Try for Ed to look at in the morning.
https://tbpl.mozilla.org/?tree=Try&rev=adc883ebe0d6

Comment 5

[Ed Morley [:emorley]]

Thank you Justin & Ryan - yeah looks good :-)

Comment 6

[Ed Morley [:emorley]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a0db0e58b509

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a0db0e58b509

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4dc64be8942d

Comment 9

[u279076]

Assuming no QA needed here. Please remove [qa-] from the whiteboard and add the verifyme keyword if this needs QA.