Closed Bug 955378 Opened 10 years ago Closed 10 years ago

Fix errors/warnings on detaching/reattaching conversations

Categories

(Instantbird Graveyard :: Conversation, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

Details

(Whiteboard: [1.4-wanted])

Attachments

(1 file, 2 obsolete files)

*** Original post on bio 1940 at 2013-04-19 11:48:00 UTC ***

> I haven't been able to reproduce this, but I do get the following warning
> 
> Timestamp: 04/12/2013 03:41:19 PM
> Warning: ReferenceError: reference to undefined property window.arguments[0]
> Source File: chrome://instantbird/content/instantbird.js
> Line: 14

This warning is harmless, it happens each time a new conv window is created by
detaching a conversation. Would still be worth filing and getting fixed (it's
trivial).

> and multiple copies of
> 
> Timestamp: 04/12/2013 03:43:14 PM
> Error: [Exception... "'JavaScript component does not have a method named:
> "supportsCommand"' when calling method: [nsIController::supportsCommand]" 
> nsresult: "0x80570030 (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)"  location:
> "JS frame :: chrome://global/content/globalOverlay.js :: goUpdateCommand ::
> line 71"  data: no]
> Source File: chrome://global/content/globalOverlay.js
> Line: 71

I have steps to reproduce for this:
1. Detach a conversation (doesn't have to be a MUC).
2. Reattach it (this closes the second window).
3. Open the context menu on the conversation content.

This is something we should fix, as I suspect it's also causing a leak. Not
1.4-blocking, but may be 1.4 wanted.
Whiteboard: [1.4-wanted]
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1940 as attmnt 2399 at 2013-04-21 16:51:00 UTC ***

The number of controllers in the contentwindow increased by one with each tab move. So it seems we should remove the existing controller before adding the new one.

I don't fully understand this code (why did it not cause issues already after only one tab detachment), hence f?
Attachment #8354166 - Flags: feedback?(florian)
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1940 as attmnt 2400 at 2013-04-21 18:23:00 UTC ***

The underlying bug was that magicCopyInitialized (along with the magic copy pref observer) was never set on detached tabs. See http://log.bezut.info/instantbird/130421/#m202
Attachment #8354167 - Flags: review?(florian)
Comment on attachment 8354166 [details] [diff] [review]
Patch

*** Original change on bio 1940 attmnt 2399 at 2013-04-21 18:23:59 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354166 - Attachment is obsolete: true
Attachment #8354166 - Flags: feedback?(florian)
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Comment on attachment 8354167 [details] [diff] [review]
Patch

*** Original change on bio 1940 attmnt 2400 at 2013-04-21 18:45:22 UTC ***

This won't work as it will try to remove nonexistent controllers etc.
Attachment #8354167 - Flags: review?(florian) → review-
Attached patch PatchSplinter Review
*** Original post on bio 1940 as attmnt 2401 at 2013-04-22 10:59:00 UTC ***

Had to spread things out over a couple of methods, but ultimately it does clean the code up a bit.
Attachment #8354168 - Flags: review?(florian)
Comment on attachment 8354167 [details] [diff] [review]
Patch

*** Original change on bio 1940 attmnt 2400 at 2013-04-22 10:59:11 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354167 - Attachment is obsolete: true
*** Original post on bio 1940 at 2013-04-23 08:45:00 UTC ***

Forgot to mention that I suspect this may also fix a bad memory leak, as the selection listener was previously not removed on destroying the convbrowser (at first glance it looks like the list of selection listeners is global, but I didn't spend much time checking the code).
Comment on attachment 8354168 [details] [diff] [review]
Patch

*** Original change on bio 1940 attmnt 2401 at 2013-05-22 20:55:25 UTC ***

I don't understand how this patch could work, so I suspect one of us is confused about what this code should be doing.

Especially, why is it possible to remove the first part of swapDocShells? And why do you just call initMagicCopy (which will just return early without doing anything) in the second part?
Attachment #8354168 - Flags: review?(florian)
*** Original post on bio 1940 at 2013-05-23 20:29:15 UTC ***

>why is it possible to remove the first part of swapDocShells?
browser.destroy() now calls that code, it's in disableMagicCopy() (see comment 5).

>why do you just call initMagicCopy (which will just return early without doing
anything) in the second part?
Why will it just return early? this.magicCopyInitialized won't generally be set in the new browser.
Comment on attachment 8354168 [details] [diff] [review]
Patch

*** Original change on bio 1940 attmnt 2401 at 2013-05-23 20:53:40 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354168 - Flags: review?(florian)
Comment on attachment 8354168 [details] [diff] [review]
Patch

*** Original change on bio 1940 attmnt 2401 at 2013-06-27 23:17:08 UTC ***

>+      <method name="uninitMagicCopy">
>+        <body>
>+          <![CDATA[
>+          if (!this.magicCopyInitialized)
>+            return;
>+          Services.prefs.removeObserver(this.magicCopyPref, this);
>+          if (this.magicCopyEnabled)
>+            this.disableMagicCopy();

Shouldn't we |this.magicCopyInitialized = false;| here?
I know it isn't actually needed, but it seems strange to me to have a method called uninitMagicCopy that puts the object in a state where initMagicCopy won't work again.


Looks pretty good by code inspection; thanks for cleaning up this mess! Not sure what was confusing me a month ago (sorry for the delay!).

I will want to test this locally on a debug build to check that nothing gets leaked before doing the checkin; but I'm pretty confident it will be fine, as I think you've already tested this patch extensively. :-)
Attachment #8354168 - Flags: review?(florian) → review+
*** Original post on bio 1940 at 2013-06-28 08:48:13 UTC ***

(In reply to comment #8)
> Shouldn't we |this.magicCopyInitialized = false;| here?
> I know it isn't actually needed, but it seems strange to me to have a method
> called uninitMagicCopy that puts the object in a state where initMagicCopy
> won't work again.

That sounds like a good idea! Will you just add it on checkin or would you like a new patch?
Whiteboard: [1.4-wanted] → [1.4-wanted][checkin-needed]
*** Original post on bio 1940 at 2013-06-29 12:23:25 UTC ***

http://hg.instantbird.org/instantbird/rev/b032f238be66
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [1.4-wanted][checkin-needed] → [1.4-wanted]
You need to log in before you can comment on or make changes to this bug.