642103 - Opening a second three-pane window and closing it breaks the UI due to the instrumentation code

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Mark Banner (:standard8)]

If I open a second 3-pane window on current trunk and then close it again, the first window's UI is completely broken (can't load different messages etc).

This appears to be a regression from the instrumentation code in bug 632639.

I think that code is trying to remove all or some observers, and failing (because you can't remove observers you haven't added, or ones you've already removed), and then somehow killing the UI completely.

Comment 1

[David :Bienvenu]

OK, I'll add a try catch somewhere (probably in the call to uninit).

Comment 2

[Mark Banner (:standard8)]

Will that actually do what you want? i.e. if I open up an additional window and then close it, is that going to kill the instrumentation stuff too early?

Comment 3

[David :Bienvenu]

(In reply to comment #2)
> Will that actually do what you want? i.e. if I open up an additional window and
> then close it, is that going to kill the instrumentation stuff too early?

The instrumentation stuff doesn't do anything once you have an account set up. It's highly unlikely that you're going to open a second 3-pane window w/o having an account set up. If we add post-account setup instrumentation, then we'll need to rework the way the instrumentation gets setup and shut down.

FWIW, I couldn't reproduce the issue you saw - what I do see is an enormous amount of output on the console that slows things down tremendously, once I open and close a second 3-pane window, even when I catch any possible exception from uninit. I'll try disabling instrumentation completely, but I suspect I'm seeing a core issue. Mine is a debug build, though.

Comment 4

[David :Bienvenu]

commenting out the loading of the instrumentation code still results in a ton of console output when I open and close a 2nd 3-pane window. The output looks like this:

###!!! ASSERTION: XPConnect is being called on a scope without a 'Components' pr
operty!  (stack and details follow): 'Error', file c:/builds/tbirdhg/mozilla/js/
src/xpconnect/src/xpcwrappednativescope.cpp, line 761
The current JS stack is:
JavaScript stack is empty
And the object whose scope lacks a 'Components' property is:
object 0BFE2168
class 5EB65240 Object
flags: branded own_shape hasPropertyTable
proto <Object at 1050D9D8>
parent <ChromeWindow object at 1050D988>
properties:

I'll see what happens with a release build.

Comment 5

[David :Bienvenu]

oops, I was building the wrong tree...try catch around uninit fixes the console output.

Comment 6

[David :Bienvenu]

Attachment: proposed fix

does this fix what you're seeing?

Comment 7

[David :Bienvenu]

Attachment: prevent exception from getting thrown...

Comment 8

[David :Bienvenu]

FWIW, I suspect that Standard8 might have been seeing an issue with the conversation add-on - if I open and close a second 3-pane window, the UI is broken, even after I fix the exception. But if I disable conversations, it's all good.

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 520080 [details] [diff] [review]
prevent exception from getting thrown...

debug builds still get very angry, console-wise, even without the conversations extension; they might not super-break though.

While the revised patch should protect us in the steady state from having to depend on your exception handler, you mentioned on IRC* that during the setup where this code is operational, you will intentionally remove observers.  I can't help but notice that these are not tracked as such and so will generate some exceptions.  This is problematic because you will likely explode when removing the "mail.accountmanager.accounts" observer and thus fail to remove the "mail.instrumentation.userOptedIn" observer.  This doesn't seem totally horrible given that preference is unlikely fire much, but it seems better to avoid it entirely.

Maybe you could create a helper method that subscribes to something and returns a function that unsubscribes, but also uses a local to track whether it has unsubbed, so it can avoid trying to unsub again and/or wrap itself in a try.

like:

let allObserverUnsubs = [];
function observeOn(what, listener) {
  observerService.observe(what, listener);
  let subscribed = true;
  let unsub = function() {
    if (!subscribed)
      return;
    subscribed = false;
    try {
      observerService.unsubscribe(what, listener);
    }
    catch {}
  }
  allObserverUnsubs.push(unsub);
  return unsub;
}

let unsubA = observeOn("foo", this), unsubB = observerOn("bar", this);
observerOn("dontcare", this);
...
unsubA();
unsubB();
allObserverUnsubs.map(function(x) {x();});


obviously I do not know how the observer service works off the top of my head...

* you sunk your own battleship!

Comment 10

[Andrew Sutherland [:asuth] (he/him)]

Attachment: add something to load when UnloadMessenger fires, and also start generating notifications

This patch improves the mozmill fancy logging so that it becomes more obvious when something bad happens during UnloadMessenger (as well as any other method we have decorated for logging purposes under mozmill.)

I have verified this does the right thing locally with a mozmill run on folder-display.

Comment 11

[Jonathan Protzenko [:protz]]

As a side-note, yes, Conversations doesn't work really well with multiple windows. I tend to not use that setup much, but that's something that should definitely be fixed :-).

Comment 12

[David :Bienvenu]

Attachment: keep track of removed prefs

this simply keeps track of which prefs we have observers for. This is only used on first run.

Comment 13

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 520213 [details] [diff] [review]
keep track of removed prefs

You should:
-  _prefsObserved : [],
+  _prefsObserved : {},
since you are not using the Array parts of the array; presumably you changed impl and forgot to update this bit.  (Obviously it works as it is.)

I ran some mozmill tests with this and it seems to make the explosions go away without triggering logException.  Hooray!

This is also definitely a lot saner looking than what I was proposing, although I was hoping if you went with my brand of overkill we ended up with a helper lib :)

Comment 14

[David :Bienvenu]

fix checked in - http://hg.mozilla.org/comm-central/rev/1e62cbf1840c

Comment 15

[Andrew Sutherland [:asuth] (he/him)]

additional mozmill logging pushed to trunk:
http://hg.mozilla.org/comm-central/rev/d1bf277c6957