Closed Bug 904628 Opened 7 years ago Closed 7 years ago

More forms.js logspam in green B2G mochitest-2 runs

Categories

(Firefox OS Graveyard :: Gaia::Keyboard, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(firefox24 unaffected, firefox25 unaffected, firefox26 fixed)

RESOLVED FIXED
Tracking Status
firefox24 --- unaffected
firefox25 --- unaffected
firefox26 --- fixed

People

(Reporter: RyanVM, Assigned: xyuan)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

+++ This bug was initially created as a clone of Bug #904224 +++

+++ This bug was initially created as a clone of Bug #902168 +++

After the two landings from bug 904224, we're still seeing lots of forms.js logspam.

https://tbpl.mozilla.org/php/getParsedLog.php?id=26488355&tree=B2g-Inbound

07:50:03     INFO -  System JS : ERROR chrome://browser/content/forms.js:843
07:50:03     INFO -                       TypeError: encoder is null
07:50:03     INFO -  ************************************************************
07:50:03     INFO -  * Call to xpconnect wrapped JSObject produced this error:  *
07:50:03     INFO -  [Exception... "'[JavaScript Error: "encoder is null" {file: "chrome://browser/content/forms.js" line: 843}]' when calling method: [nsIDOMEventListener::handleEvent]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0"  data: yes]
07:50:03     INFO -  ************************************************************

07:50:20     INFO -  System JS : ERROR chrome://browser/content/forms.js:756
07:50:20     INFO -                       TypeError: value is null

Any volunteers? :-)
Summary: More forms.js logspam in green B2G mochitest-2,3 runs → More forms.js logspam in green B2G mochitest-2 runs
I should take the bug, but I run into problems setting up mochitest for b2g dev environment on b2g-desktop and the device. I'm trying to solve it. So please feel free to take the bug if you have time.
`TypeError: encoder is null` is introduced by bug 899999. We mistakely treat the <select> object as contentEditable element and then fail to get the encoder.

`TypeError: value is null` is caused by `FormAssistant.updateSelection()`.  This method will send the focused input element info to virtual keyboard. If no input element is focused, when `FormAssistant.updateSelection()` calls `FormAssistant.getSelectionInfo()`, we get the error.
Assignee: nobody → xyuan
Status: NEW → ASSIGNED
Attached patch Fix indentation. (obsolete) — Splinter Review
Attachment #790267 - Flags: review?(fabrice)
Attachment #790263 - Attachment is obsolete: true
Attachment #790263 - Flags: review?(fabrice)
Comment on attachment 790267 [details] [diff] [review]
Fix indentation.

Review of attachment 790267 [details] [diff] [review]:
-----------------------------------------------------------------

::: b2g/chrome/content/forms.js
@@ +536,3 @@
>            target.value :
>            getContentEditableText(target);
>  

Nit: I find it more readable like this:
let value = isContentEditable(target) ? getContentEditableText(target)
                                      : target.value;

@@ +643,2 @@
>        element.value :
>        getContentEditableText(element);

Same here.

@@ +843,5 @@
>  }
>  
>  // Get the visible content text of a content editable element
>  function getContentEditableText(element) {
> +  if (!element && !isContentEditable(element)) {

Don't we want !element || !isContentEditable(element) ?
Attachment #790267 - Flags: review?(fabrice)
Attached patch bug904628Splinter Review
Fabrice, thanks.

The patch addresses all the review comments and the diff with the previous one is:

diff --git a/b2g/chrome/content/forms.js b/b2g/chrome/content/forms.js
--- a/b2g/chrome/content/forms.js
+++ b/b2g/chrome/content/forms.js
@@ -527,19 +527,18 @@ let FormAssistant = {
             requestId: json.requestId,
             selectioninfo: this.getSelectionInfo()
           });
         }
         break;
       }
 
       case "Forms:GetText": {
-        let value = !isContentEditable(target) ?
-          target.value :
-          getContentEditableText(target);
+        let value = isContentEditable(target) ? getContentEditableText(target)
+                                              : target.value;
 
         if (json.offset && json.length) {
           value = value.substr(json.offset, json.length);
         }
         else if (json.offset) {
           value = value.substr(json.offset);
         }
 
@@ -634,19 +633,18 @@ let FormAssistant = {
     sendAsyncMessage("Forms:Input", getJSON(element, this._focusCounter));
     return true;
   },
 
   getSelectionInfo: function fa_getSelectionInfo() {
     let element = this.focusedElement;
     let range =  getSelectionRange(element);
 
-    let text = !isContentEditable(element) ?
-      element.value :
-      getContentEditableText(element);
+    let text = isContentEditable(element) ? getContentEditableText(element)
+                                          : element.value;
 
     let textAround = getTextAroundCursor(text, range);
 
     let changed = this.selectionStart !== range[0] ||
       this.selectionEnd !== range[1] ||
       this.textBeforeCursor !== textAround.before ||
       this.textAfterCursor !== textAround.after;
 
@@ -839,17 +837,17 @@ function getDocumentEncoder(element) {
               Ci.nsIDocumentEncoder.OutputLFLineBreak |
               Ci.nsIDocumentEncoder.OutputNonTextContentAsPlaceholder;
   encoder.init(element.ownerDocument, "text/plain", flags);
   return encoder;
 }
 
 // Get the visible content text of a content editable element
 function getContentEditableText(element) {
-  if (!element && !isContentEditable(element)) {
+  if (!element || !isContentEditable(element)) {
     return null;
   }
 
   let doc = element.ownerDocument;
   let range = doc.createRange();
   range.selectNodeContents(element);
   let encoder = FormAssistant.documentEncoder;
   encoder.setRange(range);
Attachment #790267 - Attachment is obsolete: true
Attachment #790305 - Flags: review?(fabrice)
Attachment #790305 - Flags: review?(fabrice) → review+
https://hg.mozilla.org/integration/b2g-inbound/rev/0d349a4ea3cf

Thanks for making quick work of these, you rock!
Keywords: checkin-needed
B2G mochitest-3 is officially free of forms.js exceptions now! Unfortunately, mochitest-2 still has a bunch :(

https://tbpl.mozilla.org/php/getParsedLog.php?id=26553373&tree=B2g-Inbound

13:00:40     INFO -  3917 INFO TEST-PASS | /tests/content/html/content/test/forms/test_change_event.html | Change event shouldn't be dispatched for checkbox ---> text input type change
13:00:40     INFO -  System JS : ERROR chrome://browser/content/forms.js:645
13:00:40     INFO -                       TypeError: element is null
Whiteboard: [leave open]
Attached patch bug904628_part2Splinter Review
Sorry, I missed a check of the focused element for FormAssistant.updateSelection() within a timer callback function as following.

==========================

diff --git a/b2g/chrome/content/forms.js b/b2g/chrome/content/forms.js
--- a/b2g/chrome/content/forms.js
+++ b/b2g/chrome/content/forms.js
@@ -398,16 +398,17 @@ let FormAssistant = {
         }
 
         // Don't monitor the text change resulting from key event.
         this._ignoreEditActionOnce = true;
 
         // We use 'setTimeout' to wait until the input element accomplishes the
         // change in selection range.
         content.setTimeout(function() {
+          // Should make sure this.focusedElement is not null!
           this.updateSelection();
         }.bind(this), 0);
         break;
 
       case "keyup":
         if (!this.focusedElement) {
           break;
         }
==========================

The patch will check the focused element at the begining of FormAssistant.updateSelection, so that we won't have the same issue again.
Attachment #790672 - Flags: review?(fabrice)
Attachment #790672 - Flags: review?(fabrice) → review+
https://hg.mozilla.org/mozilla-central/rev/f107713640cb

B2G mochitest-2 is now squeaky clean! Thanks :)
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
No longer blocks: 892958
Blocks: log-SnR
Blocks: 920191
No longer blocks: log-SnR
You need to log in before you can comment on or make changes to this bug.