The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Thunderbird 12.0

Status

Thunderbird
Mail Window Front End
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

11 Branch
Thunderbird 12.0
x86_64
All

Thunderbird Tracking Flags

(thunderbird11+ fixed)

Details

Attachments

(1 attachment, 5 obsolete attachments)

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.
Blocks: 644169
tracking-thunderbird11: --- → +
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
tracking-thunderbird11: + → ---
Target Milestone: --- → Thunderbird 11.0
Created attachment 583525 [details] [diff] [review]
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
Assignee: nobody → mconley
Attachment #583525 - Flags: feedback?(sagarwal)
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
Also from Sid:

"I think we should rename it to mail.ui-rdf.version or something"
tracking-thunderbird11: --- → ?
Target Milestone: Thunderbird 11.0 → Thunderbird 12.0
Created attachment 583878 [details] [diff] [review]
Patch v2

Sid:

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

-Mike
Attachment #583525 - Attachment is obsolete: true
Attachment #583525 - Flags: feedback?(sagarwal)
Attachment #583878 - Flags: review?(sagarwal)
tracking-thunderbird11: ? → +
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.
Attachment #583878 - Flags: review?(sagarwal) → review-
Created attachment 584588 [details] [diff] [review]
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.
Attachment #583878 - Attachment is obsolete: true
Attachment #584588 - Flags: review?(sagarwal)
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.
Attachment #584588 - Flags: review?(sagarwal) → review-
Created attachment 584646 [details] [diff] [review]
Patch v4

Sid:

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

-Mike
Attachment #584588 - Attachment is obsolete: true
Attachment #584646 - Flags: review?(sagarwal)
Created attachment 584842 [details] [diff] [review]
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.
Attachment #584646 - Attachment is obsolete: true
Attachment #584646 - Flags: review?(sagarwal)
Attachment #584842 - Flags: review?(sagarwal)
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.
Attachment #584842 - Flags: review?(sagarwal) → review+
Created attachment 585024 [details] [diff] [review]
Patch v6 (r+'d by sid0)

Thanks!  Made corrections, and carried over r+.
Attachment #584842 - Attachment is obsolete: true
Comment on attachment 585024 [details] [diff] [review]
Patch v6 (r+'d by sid0)

We're definitely going to want this for TB 11.
Attachment #585024 - Flags: approval-comm-aurora?
Committed to comm-central as http://hg.mozilla.org/comm-central/rev/090cf78b2b5f
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Attachment #585024 - Flags: approval-comm-aurora? → approval-comm-aurora+
Committed to comm-aurora as http://hg.mozilla.org/releases/comm-aurora/rev/531634694cd6
status-thunderbird11: --- → fixed
You need to log in before you can comment on or make changes to this bug.