"Node was not found" error when going into appmenu->preferences->layout

RESOLVED FIXED in Thunderbird 47.0

Status

Thunderbird
Toolbars and Tabs
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: aceman, Assigned: Retro)

Tracking

Trunk
Thunderbird 47.0
x86
Linux

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

3 years ago
Appmenu->preferences->layout. As soon as the final menupopup appears (with the "classic view" etc items) the error is shown in the error console:

Error: NotFoundError: Node was not found
Source File: chrome://messenger/content/mailCore.js
Line: 306
(Assignee)

Comment 1

a year ago
Created attachment 8713950 [details] [diff] [review]
Bug_1115183.patch

Hey aceman, I may have the solution for this. 

onViewToolbarsPopupShowing was being called when you hover over preferences but also when you hover over layout. The effect is that the function is called twice with the exact same arguments but the second time it's called insertBefore can't find the node provided it then throws an error and bails out early from the function.

Adding an if check to make sure the function is only called once seems to clear up the error and does not mess up any of the menu's as far as I can tell.
(Reporter)

Comment 2

a year ago
Comment on attachment 8713950 [details] [diff] [review]
Bug_1115183.patch

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

Yes, it looks like your analysis is correct. The same function with same arguments (probably from the the same onpoupshowing attribute) is called when Preferences is opened and also when Layout is opened. That is kinda strange. But is seems the Seamonkey guys also found that out in similar situation. See
https://bugzilla.mozilla.org/show_bug.cgi?id=464653#c51 . Please fix it is in the same way as them, put the check inside onViewToolbarsPopupShowing().

::: mail/base/content/mailWindowOverlay.xul
@@ -1313,5 @@
>                     label="&preferencesCmdUnix.label;"
>  #endif
>                     oncommand="openOptionsDialog();">
>            <menupopup id="appmenu_customizeMenu"
> -                     onpopupshowing="onViewToolbarsPopupShowing(event, 'mail-toolbox');">

The patch does not apply. This line you remove is different in the tree:
http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.xul#1317
You probably have made several patches on this spot and this one is not a merge of them, just the last one.
Attachment #8713950 - Flags: feedback+

Comment 3

a year ago
This is a problem everywhere, but it usually doesn't cause a throw and so isn't visible (Bug 867393).  There, I thought the same solution as here was ok, but in fact that's hacky. Plus, the style now is to add event listeners to an element rather than on* handlers as xul attributes. Since then I had to fix this in the MoreLayouts extension as it touched the Layout menuitem; the better solution is to call event.stopImmediatePropagation() immediately in the onpopupshowing handler function, imo.
(Assignee)

Comment 4

a year ago
(In reply to alta88 from comment #3)
> the better solution is to call event.stopImmediatePropagation() immediately
> in the onpopupshowing handler function, imo.

alta88, yeah as far as bugs go this one is fairly benign. I was trying to implement the fix the way you mentioned. Immediately as I enter onViewToolbarsPopupShowing(aEvent, toolboxIds, aInsertPoint) I try calling aEvent.stopImmediatePropagation(); but that does not seem to do the trick. The function is still called when you click the submenu "Layout" 


However implementing the fix style that :aceman recommended does seem to do the trick.
Flags: needinfo?(alta88)
(Assignee)

Comment 5

a year ago
Created attachment 8714057 [details] [diff] [review]
Bug_1115183_1.patch

By the way here's the correction, I mirrored the method used in https://bugzilla.mozilla.org/show_bug.cgi?id=464653#c51 and it does fix this error without having to add an if check to the mailWindowOverlay.xul file.
Attachment #8713950 - Attachment is obsolete: true
Flags: needinfo?(acelists)
(Reporter)

Comment 6

a year ago
Comment on attachment 8714057 [details] [diff] [review]
Bug_1115183_1.patch

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

Somehow the patch does not apply again. Please update your c-c tree (but not m-c as it is broken now).

::: mail/base/content/mailCore.js
@@ +255,5 @@
>    if (!Array.isArray(toolboxIds))
>      toolboxIds = [toolboxIds];
>  
>    let popup = aEvent.target;
> +  //Return early in the event that a submenu triggers this function.

// Return early in case a submenu triggered this function via bubbling of events.

@@ +258,5 @@
>    let popup = aEvent.target;
> +  //Return early in the event that a submenu triggers this function.
> +  if (popup != aEvent.currentTarget) {
> +    return;
> +  }

The style is to not use braces for one line if blocks in JS.

Also please move this check (together with popup = event.target) to the top of the function before the toolboxIds check.
Attachment #8714057 - Flags: feedback+
(Reporter)

Updated

a year ago
Assignee: nobody → myopensourcework
Status: NEW → ASSIGNED
Flags: needinfo?(acelists)
(Assignee)

Comment 7

a year ago
Created attachment 8714084 [details] [diff] [review]
Bug_1115183_2.patch

Hm, just can't seem to catch a break with these patches messing up. I had the same problem earlier with a different bug I was working on. How about now? Does this on work?
Attachment #8714057 - Attachment is obsolete: true

Comment 8

a year ago
(In reply to Retro from comment #4)
> (In reply to alta88 from comment #3)
> > the better solution is to call event.stopImmediatePropagation() immediately
> > in the onpopupshowing handler function, imo.
> 
> alta88, yeah as far as bugs go this one is fairly benign. I was trying to
> implement the fix the way you mentioned. Immediately as I enter
> onViewToolbarsPopupShowing(aEvent, toolboxIds, aInsertPoint) I try calling
> aEvent.stopImmediatePropagation(); but that does not seem to do the trick.
> The function is still called when you click the submenu "Layout" 
> 
> 
> However implementing the fix style that :aceman recommended does seem to do
> the trick.

No, stopImmediatePropagation() goes in InitViewLayoutStyleMenu(), to prevent the event bubbling to the ancestor menupopup's handler onViewToolbarsPopupShowing(), or any other ancestor.  So the patch method is inferior to catching the problem at the root.
Flags: needinfo?(alta88)
(Assignee)

Comment 9

a year ago
Created attachment 8714183 [details] [diff] [review]
Bug_1151833.patch

My apologizes, figured alta88 meant I had to put that function call inside onViewToolbarsPopupShowing. Here's the fix that takes care of it inside InitViewLayoutStyleMenu in mailWindowOverlay.js.
Attachment #8714084 - Attachment is obsolete: true

Comment 10

a year ago
@Retro - no worries, your work on this is appreciated.
(Reporter)

Updated

a year ago
Attachment #8714183 - Flags: review?(mkmelin+mozilla)

Comment 11

a year ago
Comment on attachment 8714183 [details] [diff] [review]
Bug_1151833.patch

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

Looks good (but doesn't apply). I'll attach an unbitrotted version.

::: mail/base/content/mailWindowOverlay.js
@@ +274,5 @@
>      appmenuCharset.setAttribute("disabled", !msgWindow.mailCharacterSet);
>  }
>  
>  function InitViewLayoutStyleMenu(event)
> +{ 

nit: trailing space

@@ +275,5 @@
>  }
>  
>  function InitViewLayoutStyleMenu(event)
> +{ 
> +  //Prevent submenus from unnecessarily triggering onViewToolbarsPopupShowing via bubbling of events.

nit: we cap lines @ 80 ch
Attachment #8714183 - Flags: review?(mkmelin+mozilla) → review+

Comment 12

a year ago
Created attachment 8716462 [details] [diff] [review]
Bug_1151833.patch
Attachment #8714183 - Attachment is obsolete: true
Attachment #8716462 - Flags: review+

Updated

a year ago
Keywords: checkin-needed

Comment 13

a year ago
https://hg.mozilla.org/comm-central/rev/6fd0470fb29c33dbdc0f1f1305b392e0b625bf3a
Bug 1115183 - Prevent submenus from unnecessarily triggering onViewToolbarsPopupShowing via bubbling of events. r=mkmelin

Updated

a year ago
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 47.0
You need to log in before you can comment on or make changes to this bug.