712395 - Folder pane randomly collapses on startup with TestPilot 1.3.4 and Tabs on top

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

This is a race condition that Sid noticed while reviewing the Tabs on Top patch (bug 644169).

Steps to reproduce:

1)  With a fresh profile, install Test Pilot for Thunderbird 1.3.4 from AMO.
2)  Shut Thunderbird down.  Open up the profile's localstore.rdf file, and find the folderPaneBox description entry.
3)  For the folderPaneBox description entry, add an attribute collapsed="true".

Your folderPaneBox description entry should now look something like this:

<RDF:Description RDF:about="chrome://messenger/content/messenger.xul#folderPaneBox"
                   collapsed="true"
                   width="242" />

Now start up Thunderbird with Tabs on Top, and switch to the 3pane.

What happens:

It's a race condition, so it doesn't always happen - but a number of times, the folderPane will be collapsed by default on startup.

What's expected:

The attribute should be ignored, like it has been for years and years.

Explanation:

So localstore.rdf can potential contain a lot of fragments of old UI customization parameters (like info on toolbars that no longer exist, etc).  Sometimes, the elements that those customization parameters apply to still do exist, but the attributes are supposed to be ignored - like in the case of folderPaneBox.

What's more; when going to View > Layout, and setting the Folder Pane attribute to visible, it doesn't work the first time, only the second.

Unfortunately, this is still a poorly understood bug at this time.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

I`ve just reproduced this bug in TB 10 as well, so I don`t think Tabs-on-top is the root culprit.  I`m starting to lean towards TestPilot.

Regardless, I`ve found some handy looking code in nsBrowserGlue.js in the _migrateUI function.  We might be able to use something similar in mailGlue.js to scrub localstore.rdf of leftover fragments from old, pre-tabmail Thunderbirds.

http://mxr.mozilla.org/comm-central/source/mozilla/browser/components/nsBrowserGlue.js#1102

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP Patch v1

Sid:

Ok, here is my first crack at it.  This appears to fix the problem for me.  Can you confirmÉ  Are there any other nodes we should be worrying about besides folderPaneBox?

-Mike

Comment 3

[Mike Conley (:mconley) (:⚙️)]

From Sid0 over IRC:

(1) that should go in mailMigrator (which is meant specifically to migrate prefs and stuff)
(2) I see that we definitely want to do this before any windows open - we've been doing most migration after the 3pane opens
(3) there's a function called migrateMail in mailMigrator - please change it to migratePostAccountWizard and add a function migrateAtProfileStartup which fiddles with all the RDF stuff
(4) please add a pref called mail.ui.version

my only worry is that the name's too generic since we already do ui migration separately

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Also from Sid:

"I think we should rename it to mail.ui-rdf.version or something"

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v2

Sid:

I've performed the things you requested above.  Tested locally, and seemed to do the job flipping the collapsed attribute.

-Mike

Comment 6

[Rain [:rain1] (inactive)]

Comment on attachment 583878 [details] [diff] [review]
Patch v2

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

::: mail/base/modules/mailMigrator.js
@@ +143,5 @@
> +  /**
> +   * Determine if the UI has been upgraded in a way that requires us to reset
> +   * some user configuration.  If so, performs the resets.
> +   */
> +  _migrateUI: function MailMigrator__migrateUI() {

Let's move this up to above migratePostAccountWizard.

@@ +146,5 @@
> +   */
> +  _migrateUI: function MailMigrator__migrateUI() {
> +    // The code for this was ported from
> +    // mozilla/browser/components/nsBrowserGlue.js
> +    Cu.import("resource://gre/modules/Services.jsm");

Let's move this up to the global scope. (This actually does add Services to the global scope, so let's make the code reflect that.)

@@ +162,5 @@
> +      return;
> +
> +    this._rdf = Cc["@mozilla.org/rdf/rdf-service;1"].getService(Ci.nsIRDFService);
> +    this._dataSource = this._rdf.GetDataSource("rdf:local-store");
> +    this._dirty = false;

Ugh, I'm not happy with this at all. At the very least, please make dirty a local and set it to false whenever you call anything that modifies the data source.

@@ +195,5 @@
> +  _getPersist: function MailMigrator__getPersist(aSource, aProperty) {
> +    // The code for this was lifted verbatim from
> +    // mozilla/browser/components/nsBrowserGlue.js.
> +
> +    var target = this._dataSource.GetTarget(aSource, aProperty, true);

let

@@ +211,5 @@
> +    // mozilla/browser/components/nsBrowserGlue.js.
> +
> +    this._dirty = true;
> +    try {
> +      var oldTarget = this._dataSource.GetTarget(aSource, aProperty, true);

let

@@ +222,5 @@
> +      else {
> +        this._dataSource.Assert(aSource, aProperty, this._rdf.GetLiteral(aTarget), true);
> +      }
> +
> +      // Add the entry to the persisted set for this document if it's not there.

I don't think this is what we want... I think what you're doing is setting collapsed to false then persisting it. The uses in nsBrowserGlue.js indicate the same.

@@ +231,5 @@
> +      if (!this._dataSource.HasAssertion(docResource, persistResource, aSource, true)) {
> +        this._dataSource.Assert(docResource, persistResource, aSource, true);
> +      }
> +    }
> +    catch(ex) {}

Ew, it's worth logging the failure at least.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v3

(In reply to Siddharth Agarwal [:sid0] from comment #6)
> Comment on attachment 583878 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 583878 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mail/base/modules/mailMigrator.js
> @@ +143,5 @@
> > +  /**
> > +   * Determine if the UI has been upgraded in a way that requires us to reset
> > +   * some user configuration.  If so, performs the resets.
> > +   */
> > +  _migrateUI: function MailMigrator__migrateUI() {
> 
> Let's move this up to above migratePostAccountWizard.
> 

Done.

> @@ +146,5 @@
> > +   */
> > +  _migrateUI: function MailMigrator__migrateUI() {
> > +    // The code for this was ported from
> > +    // mozilla/browser/components/nsBrowserGlue.js
> > +    Cu.import("resource://gre/modules/Services.jsm");
> 
> Let's move this up to the global scope. (This actually does add Services to
> the global scope, so let's make the code reflect that.)
> 

Done.

> @@ +162,5 @@
> > +      return;
> > +
> > +    this._rdf = Cc["@mozilla.org/rdf/rdf-service;1"].getService(Ci.nsIRDFService);
> > +    this._dataSource = this._rdf.GetDataSource("rdf:local-store");
> > +    this._dirty = false;
> 
> Ugh, I'm not happy with this at all. At the very least, please make dirty a
> local and set it to false whenever you call anything that modifies the data
> source.
> 

In my defense, that's how I found the code. :) But yes, I agree - a local is better.  Done.

> @@ +195,5 @@
> > +  _getPersist: function MailMigrator__getPersist(aSource, aProperty) {
> > +    // The code for this was lifted verbatim from
> > +    // mozilla/browser/components/nsBrowserGlue.js.
> > +
> > +    var target = this._dataSource.GetTarget(aSource, aProperty, true);
> 
> let
> 
> @@ +211,5 @@
> > +    // mozilla/browser/components/nsBrowserGlue.js.
> > +
> > +    this._dirty = true;
> > +    try {
> > +      var oldTarget = this._dataSource.GetTarget(aSource, aProperty, true);
> 
> let
> 

Done for both.

> @@ +222,5 @@
> > +      else {
> > +        this._dataSource.Assert(aSource, aProperty, this._rdf.GetLiteral(aTarget), true);
> > +      }
> > +
> > +      // Add the entry to the persisted set for this document if it's not there.
> 
> I don't think this is what we want... I think what you're doing is setting
> collapsed to false then persisting it. The uses in nsBrowserGlue.js indicate
> the same.
> 

Ok, now we're unasserting.  Tested locally, and it indeed removes the collapsed property from the resource.

> @@ +231,5 @@
> > +      if (!this._dataSource.HasAssertion(docResource, persistResource, aSource, true)) {
> > +        this._dataSource.Assert(docResource, persistResource, aSource, true);
> > +      }
> > +    }
> > +    catch(ex) {}
> 
> Ew, it's worth logging the failure at least.

Done - with Cu.reportError.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 584588 [details] [diff] [review]
Patch v3

Marking r- for Sid0 based on an on-the-spot review in IRC.

Notes:

-Line 224 - if (aTarget) should read if (aTarget !== undefined)
-We should have an _unpersist function that unasserts a property, and also removes a resource from the persist set.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v4

Sid:

Alright, take that for a spin and let me know what you think.  Thanks!

-Mike

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v5

As per discussion in IRC, this patch does away with the _setPersist helper function.  I've also split out the _unPersist function into _unPersist and _unAssert (since we'll want to keep folderPaneBox persisted for width).

And I've added some quick Mozmill tests.  Passes on Windows 7.

Comment 11

[Rain [:rain1] (inactive)]

Comment on attachment 584842 [details] [diff] [review]
Patch v5

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

This patch looks great!

::: mail/base/modules/mailMigrator.js
@@ +230,5 @@
> +   * document. This function should only be called by _migrateUI.
> +   *
> +   * @param aSource the resource to remove from the persisted set
> +   */
> +  _unPersist: function MailMigrator__unPersist(aSource) {

This is dead code as well, let's just get rid of it.

::: mail/test/mozmill/migration-to-rdf-ui-1/localstore.rdf
@@ +30,5 @@
> +    <NC:persist RDF:resource="chrome://messenger/content/messenger.xul#abp-hooks"/>
> +    <NC:persist RDF:resource="chrome://messenger/content/messenger.xul#folderPaneBox"/>
> +  </RDF:Description>
> +  <RDF:Description RDF:about="chrome://messenger/content/messenger.xul#folderPaneBox"
> +                   width="500" />

You need the collapsed attribute here, heh.

::: mail/test/mozmill/migration-to-rdf-ui-1/test-migrate-to-rdf-ui-1.js
@@ +72,5 @@
> +  let fpbResource = rdf.GetResource(MESSENGER_DOCURL + "folderPaneBox");
> +  let collapsedResource = rdf.GetResource("collapsed");
> +  let target = datasource.GetTarget(fpbResource, collapsedResource, true);
> +
> +  if (target instanceof Ci.nsIRDFLiteral)

target != null maybe? It theoretically could be something other than an RDF literal too.

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v6 (r+'d by sid0)

Thanks!  Made corrections, and carried over r+.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 585024 [details] [diff] [review]
Patch v6 (r+'d by sid0)

We're definitely going to want this for TB 11.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Committed to comm-central as http://hg.mozilla.org/comm-central/rev/090cf78b2b5f

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Committed to comm-aurora as http://hg.mozilla.org/releases/comm-aurora/rev/531634694cd6