Closed Bug 916358 Opened 8 years ago Closed 5 years ago

[Exception... "'[JavaScript Error: "this._settings.contentDocument.body is undefined" {file: "chrome://messenger/content/cloudfile/addAccountDialog.js" line: 118}]' when calling method: [nsIDOMEventListener::handleEvent]" nsresult: "0x80570021 (NS_ERROR_

Categories

(Thunderbird :: Account Manager, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 52.0

People

(Reporter: ishikawa, Assigned: ishikawa)

References

Details

Attachments

(2 files, 3 obsolete files)

(Apologies if this bug entry is entered twice. I thought I entered this several hours ago, but now I notice that it is not yet in bugzilla. Now I realize a mistake: Due to a local copy and paste mistake, I may have gobbled together the reports of two bug entries in a mixed manner :-(. 
Bug 912172 comment 2 is meant for this different  bug entry! (and there was a paragraph in there which was meant for the original 912172. Sorry about the mix up.)

Here is a new entry for this bug with the correct subject.

I see the following error during the run of |make mozmill| test suite for TB.

[Exception... "'[JavaScript Error: "this._settings.contentDocument.body is undefined" {file: "chrome://messenger/content/cloudfile/addAccountDialog.js" line: 118}]' 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]

Attached is the excerpt from the log.

In the second post, I will explain the patch.

PS: Sorry for the confusion of posting mixed-up reports of two
different bugs.
Attached patch Proposed fix (take one) (obsolete) — Splinter Review
Patch synopsis:

    Fix for "this._settings.contentDocument.body is undefined"

    |body| is checked before use, and a logic was changed so that
    window.sizeToContent() is not called any more when |body| is null.

Detail:

In addAccountDialog.js, |.body| is referenced on line 118 as follows.

	      fitIFrame: function() {
	// Determine the height of the accountSettings iframe, and adjust
	// the height of the window appropriately.
-->	let newHeight = this._settings.contentDocument
				      .body
				      .offsetHeight;
	this._settings.style.height = this._settings.style.minHeight = newHeight + "px";
	window.sizeToContent();
      },
	  
1st Observation:

After generating the error with the test target
test-cloudfile-add-account-dialog.js |
test-cloudfile-add-account-dialog.js::test_accept_disabled_if_no_services
I realize that the test produced error when there is no account
information available.

If no account is available, probably content for it is null.  It is no
wonder .body is undefined.

But then, we have no intention of showing the content (empty) for the
user to see.  So I think we can safely change the newHeight to zero or
1 (one) when this._settings.contentDocument is null.

      So I conditionally set the newHeight to a fallback value when
      |.body| is undefined in my first attempted patch. 
      
      I ran |make mozmill| with this 1st change.

2nd Observation:

However, It turns out simply setting some fallback value and continue
calling window.sizeToContent() eventually leads to TB crash in gtk
window redraw code. (I think something goes wrong with
window.sizeToContent() when actually the |.body| is null under the
circumstances.

So I modified the code to set the default height, etc. with a fallback
value, but never to call window.sizeToContent() if |.body| property is
undefined.  

I thought all was well by visually monitoring what is shown on the
screen: I inserted enough delay (2sec) in the test code to see the
displayed dialog on the screen. (Without such delay it is drawn for a
split second without even having the chance to be filled with proper
messages, buttons, and disappear quickly.)

TIA

PS: Sorry for the confusion of posting mixed-up reports of two
different bugs.
I confirm I also see this error.
Attachment #804781 - Flags: review?(mconley)
Assignee: nobody → ishikawa
Status: NEW → ASSIGNED
Blocks: 826967
Comment on attachment 804781 [details] [diff] [review]
Proposed fix (take one)

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

Thanks! I just have a few suggestions.

::: mail/components/cloudfile/content/addAccountDialog.js
@@ +127,5 @@
> +    // but never call window.sizeToContent() in this case.
> +    // Throwing an error value was not quite simple to handle.
> +    // It is difficult to figure out where we should catch the error.
> +
> +    if(!this._settings.contentDocument.body) {

Instead of having fitIFrame return if no body, we should just never call it - in the onInit function, after addAccountTypes, we should determine if any account types were added to the menulist. If there weren't, we should just return early in there, and not deal with any of the stuff after addAccountTypes.

::: mail/test/mozmill/cloudfile/test-cloudfile-add-account-dialog.js
@@ +125,5 @@
>   */
>  function test_accept_disabled_if_no_services() {
>    assert_num_providers(0);
>    plan_for_modal_dialog(kDialogId, subtest_accept_disabled_if_no_services);
> +  mc.sleep(2000);    

Please remove the sleeps.
Attachment #804781 - Flags: review?(mconley) → review-
Could this be reworked per reviewer's comments?
Flags: needinfo?(ishikawa)
(In reply to :aceman from comment #4)
> Could this be reworked per reviewer's comments?

Will do.
Flags: needinfo?(ishikawa)
I did the following:

--- begin quote ---
Instead of having fitIFrame return if no body, we should just never call it - in the onInit function, after addAccountTypes, we should determine if any account types were added to the menulist. If there weren't, we should just return early in there, and not deal with any of the stuff after addAccountTypes.
--- end quote ---

But I left the error message print out in fitIFrame just in case.
Well, the error message gets printed very often now during |make mozmill| test suite run under linux.

Looking at the code, I see fitIFrame may be called in this handler in the same .js file, too.

handleEvent: function(aEvent) {
    switch (aEvent.type) {
      case "DOMContentLoaded": {
        this.onIFrameLoaded();
        break;
      }
      case "overflow": {
        this.fitIFrame();
        break;
      }
      case "select": {
        this.accountTypeSelected();
        break;
      }
    }
  },

Can it be that we should NOT call fitIframe if this._settings.contentDocument.body is null in the handler ???
In addition to skipping calling fitIFrame() when no item is added to the menulist, 
the call to |fitIFrame()| in a handler is now done only if
this._settings.contentDocument.body is defined and not null.

Now the log does not have the original error messages at all!

(I left the check and error message in |fitIFrame()| to notice future regression quickly.)

TIA
Attachment #804781 - Attachment is obsolete: true
Attachment #8794469 - Flags: review?(mconley)
Without this patch, we see the error messages:
For example, I will pick up aceman's try-comm-central job.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=bc7d1957627e720c6f29d88d0e8e68a2dd1d1a42

We see the following error in, say, linux debug mozmill raw log.

06:26:33     INFO -  JavaScript error: chrome://messenger/content/cloudfile/addAccountDialog.js, line 150: TypeError: this._settings.contentDocument.body is undefined

https://archive.mozilla.org/pub/thunderbird/try-builds/acelists@atlas.sk-bc7d1957627e/try-comm-central-linux-debug/try-comm-central_ubuntu32_vm-debug_test-mozmill-bm01-tests1-linux32-build0.txt.gz

However, with my patch, the error does not get printed at all.
For example,
try-comm-central submission:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=fc9cd96af18df36aa7d0123556e577c1a14d5b8d

This patch is in:
https://hg.mozilla.org/try-comm-central/rev/e2f890aec6dd

raw log for linux debug run of  mozill test suite:
https://archive.mozilla.org/pub/thunderbird/try-builds/ishikawa@yk.rim.or.jp-fc9cd96af18d/try-comm-central-linux64-debug/try-comm-central_ubuntu64_vm-debug_test-mozmill-bm121-tests1-linux64-build12.txt.gz

We can't find the string ".body" in the raw log at all.

TIA
Comment on attachment 8794469 [details] [diff] [review]
Proposed fix (take two): skip calling |fitIFrame()| when this._settings.contentDocument.body is undefined

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

Thanks! I have a few comments - see below.

::: mail/components/cloudfile/content/addAccountDialog.js
@@ +74,5 @@
> +
> +    // We should determine if any account types were added to the menulist.
> +    // If there weren't, we should just return early here, and not deal with the
> +    // other stuff.
> +    if ( this.addAccountTypes() == 0)

No space after (.

Could also be:

if (!this.addAccountTypes())

@@ +159,5 @@
> +    // When no account is available, we get undefined |.body|, and our
> +    // possible choices are
> +    // - provide a fallback value.
> +    // - throw an error value signifying a special case.
> +    // providing a fallback value later results in crash. TB is not

Results in a crash? Can I see a stack? Or do you mean something other than a process crash?

@@ +160,5 @@
> +    // possible choices are
> +    // - provide a fallback value.
> +    // - throw an error value signifying a special case.
> +    // providing a fallback value later results in crash. TB is not
> +    // quite written that way. So we set the fallback value to style.heigh and such,

typo: "heigh" -> "height"

@@ +165,5 @@
> +    // but never call window.sizeToContent() in this case.
> +    // Throwing an error value was not quite simple to handle.
> +    // It is difficult to figure out where we should catch the error.
> +
> +    if(!this._settings.contentDocument.body) {

Space after if

@@ +166,5 @@
> +    // Throwing an error value was not quite simple to handle.
> +    // It is difficult to figure out where we should catch the error.
> +
> +    if(!this._settings.contentDocument.body) {
> +      dump("WARNING: addAccountDialog.js: fitFrame: There is no account, and this._settings.contentDocument.body was undefined. \n");

We shouldn't dump in production codepaths. Cu.reportError might make more sense.

@@ +167,5 @@
> +    // It is difficult to figure out where we should catch the error.
> +
> +    if(!this._settings.contentDocument.body) {
> +      dump("WARNING: addAccountDialog.js: fitFrame: There is no account, and this._settings.contentDocument.body was undefined. \n");
> +      this._settings.style.height = this._settings.style.minHeight = 16 + "px";

Where is this magic number coming from? Wouldn't 0 make more sense to hide the settings area entirely?

@@ +168,5 @@
> +
> +    if(!this._settings.contentDocument.body) {
> +      dump("WARNING: addAccountDialog.js: fitFrame: There is no account, and this._settings.contentDocument.body was undefined. \n");
> +      this._settings.style.height = this._settings.style.minHeight = 16 + "px";
> +      return; 	  // withtout calling window.sizeToContent();

typo: "withtout" -> "without"

@@ +189,2 @@
>    addAccountTypes: function AAD_addAccountTypes() {
> +    let maddcount = 0;

We can probably choose a clearer variable name. Perhaps accountTypeTotal?
Attachment #8794469 - Flags: review?(mconley) → review-
(In reply to Mike Conley (:mconley) - (needinfo me!) from comment #9)
> > +    if ( this.addAccountTypes() == 0)
> 
> No space after (.
> 
> Could also be:
> 
> if (!this.addAccountTypes())

No, we prefer '== 0' when the function returns an integer.

> > +    if(!this._settings.contentDocument.body) {
> > +      dump("WARNING: addAccountDialog.js: fitFrame: There is no account, and this._settings.contentDocument.body was undefined. \n");
> > +      this._settings.style.height = this._settings.style.minHeight = 16 + "px";
> 
> Where is this magic number coming from? Wouldn't 0 make more sense to hide
> the settings area entirely?

Or some .hidden/.collapsed = true, if it is a XUL element.
Thank you for the reviews.

Firstly:

(In reply to Mike Conley (:mconley) - (needinfo me!) from comment #9)
> Comment on attachment 8794469 [details] [diff] [review]
> Proposed fix (take two): skip calling |fitIFrame()| when
> this._settings.contentDocument.body is undefined
> 
> Review of attachment 8794469 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks! I have a few comments - see below.
> 
> ::: mail/components/cloudfile/content/addAccountDialog.js
> @@ +74,5 @@
> > +
> > +    // We should determine if any account types were added to the menulist.
> > +    // If there weren't, we should just return early here, and not deal with the
> > +    // other stuff.
> > +    if ( this.addAccountTypes() == 0)
> 
> No space after (.

Done.

> 
> Could also be:
> 
> if (!this.addAccountTypes())

Based on aceman's followup comment, I did not change this part.

> 
> @@ +159,5 @@
> > +    // When no account is available, we get undefined |.body|, and our
> > +    // possible choices are
> > +    // - provide a fallback value.
> > +    // - throw an error value signifying a special case.
> > +    // providing a fallback value later results in crash. TB is not
> 
> Results in a crash? Can I see a stack? Or do you mean something other than a
> process crash?

Yes, it was quite a while ago so can't recall the details, but I think the crash occurred in sizeToContent() call.  

> @@ +160,5 @@
> > +    // possible choices are
> > +    // - provide a fallback value.
> > +    // - throw an error value signifying a special case.
> > +    // providing a fallback value later results in crash. TB is not
> > +    // quite written that way. So we set the fallback value to style.heigh and such,
> 
> typo: "heigh" -> "height"

Fixed. 

> 
> @@ +165,5 @@
> > +    // but never call window.sizeToContent() in this case.
> > +    // Throwing an error value was not quite simple to handle.
> > +    // It is difficult to figure out where we should catch the error.
> > +
> > +    if(!this._settings.contentDocument.body) {
> 
> Space after if

Fixed.

> 
> @@ +166,5 @@
> > +    // Throwing an error value was not quite simple to handle.
> > +    // It is difficult to figure out where we should catch the error.
> > +
> > +    if(!this._settings.contentDocument.body) {
> > +      dump("WARNING: addAccountDialog.js: fitFrame: There is no account, and this._settings.contentDocument.body was undefined. \n");
> 
> We shouldn't dump in production codepaths. Cu.reportError might make more
> sense.
> 

Now use cu.reportError().

> @@ +167,5 @@
> > +    // It is difficult to figure out where we should catch the error.
> > +
> > +    if(!this._settings.contentDocument.body) {
> > +      dump("WARNING: addAccountDialog.js: fitFrame: There is no account, and this._settings.contentDocument.body was undefined. \n");
> > +      this._settings.style.height = this._settings.style.minHeight = 16 + "px";
> 
> Where is this magic number coming from? Wouldn't 0 make more sense to hide
> the settings area entirely?

I thought I might need to pass a reasonable value for a single character height or something.
 
> @@ +168,5 @@
> > +
> > +    if(!this._settings.contentDocument.body) {
> > +      dump("WARNING: addAccountDialog.js: fitFrame: There is no account, and this._settings.contentDocument.body was undefined. \n");
> > +      this._settings.style.height = this._settings.style.minHeight = 16 + "px";
> > +      return; 	  // withtout calling window.sizeToContent();
> 
> typo: "withtout" -> "without"

Fixed.

> 
> @@ +189,2 @@
> >    addAccountTypes: function AAD_addAccountTypes() {
> > +    let maddcount = 0;
> 
> We can probably choose a clearer variable name. Perhaps accountTypeTotal?

Done using the suggested variable name.

Now as for aceman's comment:

> > > +    if(!this._settings.contentDocument.body) {
> > +      dump("WARNING: addAccountDialog.js: fitFrame: There is no account, and this._settings.contentDocument.body was undefined. \n");
> > +      this._settings.style.height = this._settings.style.minHeight = 16 + "px";
> > 
> > Where is this magic number coming from? Wouldn't 0 make more sense to hide
> > the settings area entirely?

> Or some .hidden/.collapsed = true, if it is a XUL element.

Then it seemed that the purported intention when there is no account
is not to show the setting area completely.

I have a feeling that 
- since this is no longer called (hopefully) when the account is not available,
  the discussion is moot, AND
- if, for some reason (by future code changes for example), this function gets called, it would be better to show an empty/unfilled setting area to make the user REALIZE that something is amiss: maybe then s/he probably looks at the error console (which is pretty much hidden and I don't like it very much) and decides to take corrective actions (if they are easy to figure out.).

I wonder what others think.
Attachment #8794469 - Attachment is obsolete: true
Attachment #8796845 - Flags: review?(mconley)
The crash was explained in comment 1: 
> 2nd Observation:
>
> However, It turns out simply setting some fallback value and continue
> calling window.sizeToContent() eventually leads to TB crash in gtk
> window redraw code. (I think something goes wrong with
> window.sizeToContent() when actually the |.body| is null under the
> circumstances.
Comment on attachment 8796845 [details] [diff] [review]
Proposed fix (take three): skip calling |fitIFrame()| when this._settings.contentDocument.body is undefined

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

Thanks!
Attachment #8796845 - Flags: review?(mconley) → review+
Thank you.
I re-created the patch with "r=mconley" added in the header line at the end.
I will put checkin-needed keyword.
Attachment #8796845 - Attachment is obsolete: true
Attachment #8799022 - Flags: review+
Attachment #8799022 - Attachment description: Proposed fix (take three): skip calling |fitIFrame()| when this._settings.contentDocument.body is undefined. r=mconley → Proposed fix (take three): skip calling |fitIFrame()| when this._settings.contentDocument.body is undefined. Carrying over r=mconley
Keywords: checkin-needed
Comment on attachment 8799022 [details] [diff] [review]
Proposed fix (take three): skip calling |fitIFrame()| when this._settings.contentDocument.body is undefined. Carrying over r=mconley

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

Please fix the comments before landing this.
Rules for comments:
1) Explain why you're doing something more than what you're doing.
2) No endless deliberations. You can mention briefly other options that didn't work.
3) No "should" and "let us" and no inappropriate language.
   English grammar and punctuation rules apply.
4) Keep it short and succinct.
5) Mark future work with XXX.

::: mail/components/cloudfile/content/addAccountDialog.js
@@ +73,5 @@
>      this.removeTitleMenuItem();
> +
> +    // We should determine if any account types were added to the menulist.
> +    // If there weren't, we should just return early here, and not deal with the
> +    // other stuff.

No conversation style comment:
Hey Joe, we really should fix this one day.

This comment should read:
Determine if any account types were added to the menulist, if not, return early here.
"Stuff" is inappropriate language.

@@ +156,5 @@
>      // Determine the height of the accountSettings iframe, and adjust
>      // the height of the window appropriately.
> +
> +    // When no account is available, we get undefined |.body|, and our
> +    // possible choices are

Nit: No comma before and and a colon (:) after are.

@@ +159,5 @@
> +    // When no account is available, we get undefined |.body|, and our
> +    // possible choices are
> +    // - provide a fallback value.
> +    // - throw an error value signifying a special case.
> +    // providing a fallback value later results in crash. TB is not

This should be upper case.

@@ +164,5 @@
> +    // quite written that way.
> +    // So we set the fallback value to style.height and such,
> +    // but never call window.sizeToContent() in this case.
> +    // Throwing an error value was not quite simple to handle.
> +    // It is difficult to figure out where we should catch the error.

Please cut this conversational comment. I don't see what is deliberation that doesn't belong here and what is documenting the code. Please only explain why you are doing things.

@@ +167,5 @@
> +    // Throwing an error value was not quite simple to handle.
> +    // It is difficult to figure out where we should catch the error.
> +
> +    if (!this._settings.contentDocument.body) {
> +      Cu.reportError("WARNING: addAccountDialog.js: fitFrame: There is no account, and this._settings.contentDocument.body was undefined.");

Nits: No comma before "and" and s/was/is/.

@@ +168,5 @@
> +    // It is difficult to figure out where we should catch the error.
> +
> +    if (!this._settings.contentDocument.body) {
> +      Cu.reportError("WARNING: addAccountDialog.js: fitFrame: There is no account, and this._settings.contentDocument.body was undefined.");
> +      this._settings.style.height = this._settings.style.minHeight = 16 + "px";

What is 16 + "px". That's "16px". Why does the runtime have to work this out?

@@ +169,5 @@
> +
> +    if (!this._settings.contentDocument.body) {
> +      Cu.reportError("WARNING: addAccountDialog.js: fitFrame: There is no account, and this._settings.contentDocument.body was undefined.");
> +      this._settings.style.height = this._settings.style.minHeight = 16 + "px";
> +      return; // without calling window.sizeToContent();

Please remove this obvious comment.

@@ +185,5 @@
>      }
>    },
>  
> +  // let us return 0 if no addition to menulist happens,
> +  // and the number of items if addition(s) to menulist happened.

I really dislike this conversational style "let us".
This should read:
Return 0 if no addition to menulist happened or
the number of additions otherwise.
Besides, "let" needs to be uppercase.
This is NOT ready for check-in.

Reviewers, can you please ensure that the quality of the comments formally meets our coding standard and that the comment content is as precise as possible.
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/1ffc8607e550c1acaf0fca3d552cb2782f40a436
Landed with comments fixed.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 52.0
(In reply to Jorg K (GMT+2) from comment #17)
> https://hg.mozilla.org/comm-central/rev/
> 1ffc8607e550c1acaf0fca3d552cb2782f40a436
> Landed with comments fixed.
Thank you for fixing the comments and landing it.
I thought I would do it today, a national holiday in Japan, but you beat me to it ;-)
You need to log in before you can comment on or make changes to this bug.