columnpicker restore column order doesn't work anymore

RESOLVED FIXED in Firefox 67

Status

()

defect
P3
normal
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: mkmelin, Assigned: vporof)

Tracking

unspecified
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

3 months ago

+++ 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.

Updated

3 months ago
Flags: needinfo?(vporof)
Assignee

Comment 1

3 months ago

Investigating now.

Assignee

Updated

3 months ago
Assignee: nobody → vporof
Flags: needinfo?(vporof)

Comment 2

3 months ago

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.

Assignee

Comment 3

3 months ago

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

Comment 4

3 months ago

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.

Comment 6

3 months ago

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).

Comment 8

3 months ago

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.

Assignee

Comment 10

3 months ago

(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.

Comment 12

2 months ago

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.

Assignee

Comment 13

2 months ago

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.

Comment 14

2 months ago
Pushed by vporof@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2883b91c8f6f
Columnpicker restore column order doesn't work anymore, r=smaug

Comment 15

2 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
You need to log in before you can comment on or make changes to this bug.