columnpicker restore column order doesn't work anymore

RESOLVED FIXED in Firefox 67

Status

()

defect
P3
normal
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: mkmelin, Assigned: vporof)

Tracking

unspecified
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(1 attachment)

+++ This bug was initially created as a clone of Bug #1523957 +++

Since bug 1523957 the "Restore column order" command doesn't work. At least the time frame fits, so I'm pretty sure it's from there.

This can be seen at least by going to View Certificates in the preferences. Drag a column, and try the restore command (in the column picker menu). Nothing.

Flags: needinfo?(vporof)

Investigating now.

Assignee: nobody → vporof
Flags: needinfo?(vporof)

And news here? The conversion of the trees to CE has broken the restore feature, caused a crash (bug 1530207) and forced us to turn off 10 tests in Thunderbird in bug 1529872 which reflect the bustage here, the crash and further failures. It would be good to return the Mozilla platform code back to a working state so we can see which functional issues we still need to address.

Yup, I've identified the specific issue with the columnpicker and expect to have a fix in before merge.

Hi Victor, which "merge" are you referring to? Hopefully not the next "merge day" on 2019-03-18 ;-)

Meanwhile bug 1531282 fixed four of the 10 failing tests from bug 1529872, six are left due to this bug here, all related to restoring. The crash from bug 1530207 should also be fixed by bug 1531282, at least I don't see the crash during our test run any more.

(In reply to Jorg K (GMT+1) from comment #4)

Hi Victor, which "merge" are you referring to? Hopefully not the next "merge day" on 2019-03-18 ;-)

Yes, that's the next merge day.

Meanwhile bug 1531282 fixed four of the 10 failing tests from bug 1529872, six are left due to this bug here, all related to restoring. The crash from bug 1530207 should also be fixed by bug 1531282, at least I don't see the crash during our test run any more.

Sounds like it's stabilizing already quite a bit, that's nice. Moving tree from XBL to CE was a pretty big change, so some fallout was expected.

Um, we have to wait 17 days?

Having this issue in nightly Firefox builds for a couple weeks frankly isn't that big of a deal. This feature isn't used in any of the primary user-facing trees (places UI, sidebar, etc), and I haven't seen any duplicate reports or other complaints.

It'll be fixed for 67, and quite possibly earlier depending on what the fix is. If that's too long to wait for you, then feel free to grab it first. My best guess (having not debugged it) is that nsTreeColumns::RestoreNaturalOrder is getting confused by the columns being slotted into Shadow DOM instead of XBL (https://searchfox.org/mozilla-central/rev/00f3836a87b844b5e4bc82f698c559b9966e4be2/layout/xul/tree/nsTreeColumns.cpp#410).

Sadly we have enough other work. As you know, the tree stuff is on prime UI in TB.

Meanwhile I discovered that the crash from bug 1530207 is still there, I cheered too early. It's a funny one, one test by itself doesn't crash, but it does crash when run in the context of others. I can't even see how this relates to trees.

(In reply to (Unavailable until March 11) Brian Grinstead [:bgrins] from comment #7)

Having this issue in nightly Firefox builds for a couple weeks frankly isn't that big of a deal. This feature isn't used in any of the primary user-facing trees (places UI, sidebar, etc), and I haven't seen any duplicate reports or other complaints.

It'll be fixed for 67, and quite possibly earlier depending on what the fix is. If that's too long to wait for you, then feel free to grab it first. My best guess (having not debugged it) is that nsTreeColumns::RestoreNaturalOrder is getting confused by the columns being slotted into Shadow DOM instead of XBL (https://searchfox.org/mozilla-central/rev/00f3836a87b844b5e4bc82f698c559b9966e4be2/layout/xul/tree/nsTreeColumns.cpp#410).

Indeed, that's what I said I found when initially looking into this patch, and confirmed with Emilio. Attached a patch which fixes things.

Ready to land? As I said, we have nine test failures due to this and while fixing the restore capability will fix some tests, we still need to investigate the crash mentioned in bug 1530207, see for example here: bp-14edc8a3-7589-4c02-8d67-206a70190310 with only M-C code in the call stack.

Oh, yes, this was r+ed last week, and Lando-ed it. Looks like it didn't go through for some reason, landing again now.

Pushed by vporof@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2883b91c8f6f
Columnpicker restore column order doesn't work anymore, r=smaug
Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
You need to log in before you can comment on or make changes to this bug.