Closed Bug 1472494 Opened 3 years ago Closed 3 years ago

Opening message in new tab gives errors in debug console

Categories

(Thunderbird :: General, defect)

defect
Not set
normal

Tracking

(thunderbird62 fixed, thunderbird63 fixed)

RESOLVED FIXED
Thunderbird 63.0
Tracking Status
thunderbird62 --- fixed
thunderbird63 --- fixed

People

(Reporter: jorgk-bmo, Assigned: darktrojan)

References

Details

Attachments

(1 file, 1 obsolete file)

[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIWebProgress.addProgressListener]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"
location: "JS frame :: chrome://messenger/content/tabmail.xml :: openTab :: line
 621"  data: no]
-- Exception object --
+ toString (function) 3 lines
+ name (string) 'NS_ERROR_FAILURE'
+ message (string) 'Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIWebProgress.addProgressListener]'
+ result (number) 2147500037
+ filename (string) 'chrome://messenger/content/tabmail.xml'
+ lineNumber (number) 621
+ columnNumber (number) 0
+ data (object) null
+ stack (string) 'openTab@chrome://messenger/content/tabmail.xml:621:13
OpenMessageInNewTab@chrome://messenger/content/msgMail3PaneWindow.js:1155:3
oncommand@chrome://messenger/content/messenger.xul:1:1
'
+ location (object) JS frame :: chrome://messenger/content/tabmail.xml :: openTab :: line 621
*
-- Stack Trace --
openTab@chrome://messenger/content/tabmail.xml:621:13
OpenMessageInNewTab@chrome://messenger/content/msgMail3PaneWindow.js:1155:3
oncommand@chrome://messenger/content/messenger.xul:1:1

Looks like a regression from our WE stuff, bug 1455471.
Flags: needinfo?(philipp)
Flags: needinfo?(geoff)
Richard, do you see this in a Daily?
Flags: needinfo?(richard.marti)
You can see this on automation as well, for example here:
https://taskcluster-artifacts.net/ZwfrfnpuSLO8FQn3oa7Tww/0/public/logs/live_backing.log
Yes, I see this too. Interestingly the first time it shows no error but opens the tab in background. And from the second time it shows the error ...and opens the tab also in background.
Flags: needinfo?(richard.marti)
Attached patch 1472494-newtab-error-1.diff (obsolete) — Splinter Review
I mentioned this in bug 1455471 comment 16. Now I understand what the cause is. A "new" tab of some times isn't new at all, it's the same tab reused for the new purpose. So adding the listener a second time raises an exception. I think we can safely catch the exception and move on.
Assignee: nobody → geoff
Status: NEW → ASSIGNED
Flags: needinfo?(geoff)
Attachment #8989098 - Flags: review?(jorgk)
Comment on attachment 8989098 [details] [diff] [review]
1472494-newtab-error-1.diff

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

Hmm, is there no better way to do this? Can't we track whether we've already attached a listener? For example, in MsgComposeCommands.js we have a dictionaryRemovalObserver with an 'isAdded' property. Could something similar be done here? I haven't looked at all.

::: mail/base/content/tabmail.xml
@@ +619,5 @@
>              // support, so let's assume one browser only for now.
> +            try {
> +              browser.webProgress.addProgressListener(this, Ci.nsIWebProgress.NOTIFY_ALL);
> +            } catch (ex) {
> +              if (ex.result != Components.results.NS_ERROR_FAILURE) { // Listener already added.

This should be Cr., no? We've been replacing with Ci., Cu. and Cr. all over.
Better?
Attachment #8989098 - Attachment is obsolete: true
Attachment #8989098 - Flags: review?(jorgk)
Flags: needinfo?(philipp)
Attachment #8989132 - Flags: review?(jorgk)
Comment on attachment 8989132 [details] [diff] [review]
1472494-newtab-error-2.diff

(In reply to Geoff Lankow (:darktrojan) from comment #6)
> Better?
Well, you tell me. I think it's always better to consciously control than to deal with the fallout of random events.
Attachment #8989132 - Flags: review?(jorgk) → review+
Landing tonight after the next M-C merge. You know the game ;-)
Keywords: checkin-needed
Does with this patch the tab open again in front when opening it (see comment 3)? Or am I the only there this happens?
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/7afe77cf1461
don't attach progress listener more than once. r=jorgk
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Umm, "Open Message in New Tab" always seems to open in a non-active tab. Am I missing something or are you?
Target Milestone: --- → Thunderbird 63.0
Tested now also on TB 52 as reference. Yes double click opens in front and through menu in background.
Attachment #8989132 - Flags: approval-comm-beta+
Blocks: 1455471
You need to log in before you can comment on or make changes to this bug.