608687 - <rule>-generated toolbar items are cloned when leaving toolbar customize

Status: VERIFIED FIXED

(Toolkit :: Toolbars and Toolbar Customization, defect)

Description

[Manuel Reimer]

So far (Firefox 3.6), RDF-rule-generated toolbar items just were left in an "empty" state and the datasource was removed, when toolbar-customize-mode was left.

With the latest Firefox trunk, RDF-rule-generated toolbar items are cloned including all children. If I just reconnect datasource after toolbar customization and call "builder.rebuild()", then all items are duplicated. One instance which is *not* controlled by the builder and one instance controlled by the builder. I have to workaround this by running over all children and removing them.

Expected behaviour: A RDF-rule generated toolbar item should either be placed in functional state (with datasource connected) or empty as in Firefox 3.6.

To reproduce, install PrefBar (addons.mozilla.org), go to toolbar customize mode, leave toolbar customize mode and then have a look at PrefBar. I've linked to the PrefBar bug, caused by this one, via URL-field of this bug.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Manuel, it would be good to know when it has been regressed. CC'ing Dao who probably knows a bug which could have regressed it.

Comment 2

[Sascha Grage]

Firefox:
 last good nightly: 20101026 (Windows NT 6.1; WOW64; rv:2.0b8pre)
 first bad nightly: 20101028 (Windows NT 6.1; WOW64; rv:2.0b8pre)

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Thanks, can you also please tell us the changeset id in about:buildconfig for both of those builds?

Comment 4

[Sascha Grage]

good d253c44465ae
bad  fe4898e97431

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d253c44465ae&tochange=fe4898e97431

Hm, there is not clearly an obvious bug, but could it be a regression from the Tracemonkey merge?

Comment 6

[Dietrich Ayala (:dietrich)]

Blocking for investigation.

Comment 7

[Asaf Romano (gone)]

Backing out the checkin of http://hg.mozilla.org/mozilla-central/rev/3e08f8844f87 fixes this bug.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #7)
> Backing out the checkin of
> http://hg.mozilla.org/mozilla-central/rev/3e08f8844f87 fixes this bug.

CC'ing Neil, who wrote this patch.

Comment 9

[Neil Deakin]

(In reply to comment #0)
> With the latest Firefox trunk, RDF-rule-generated toolbar items are cloned
> including all children. If I just reconnect datasource after toolbar
> customization and call "builder.rebuild()", then all items are duplicated. One
> instance which is *not* controlled by the builder and one instance controlled
> by the builder. I have to workaround this by running over all children and
> removing them.
> 

Could you describe where in the code this is happening?

Or, better, just provide a short block of code which shows what you are doing to create this bug?

Comment 10

[Daniel Veditz [:dveditz]]

Does this happen on 3.6.13pre as well? AFAIK we took the same patch there.

Comment 11

[[:Cww]]

(In reply to comment #10)
Installed prefbar in a clean profile, can confirm it happens with the 3.6.13 build 2 candidate.

Comment 12

[Daniel Veditz [:dveditz]]

The regressing bug was backed out of the 1.9.2 branch

Comment 13

[Manuel Reimer]

The code, affected by this bug, starts here:
http://git.tuxfamily.org/?p=gitroot/prefbar/main.git;a=blob;f=content/prefbarOverlay.js;h=95e4b7766ea13f1d786ed220d2870053fec16d75;hb=HEAD#l201

But anything, I do here, is to reconnect the datasource.

I think the following is happening: When customizing the toolbar, Firefox moves the toolbar items around. So far this caused the datasource to be dropped. Means, that rule-generated menus or toolbaritems were just empty. By reconnecting the datasource and calling "builder.rebuild()" anything worked again.

With this regression bug, the "moving" of the items still disconnects the datasource, but the items are not longer dropped. Instead of empty menus/toolbaritems, they are now still filled. When reconnecting the datasource and calling "builder.rebuild()", the items are now duplicated. One old instance and one new instance, created by the ruleset.

Comment 14

[Manuel Reimer]

> The regressing bug was backed out of the 1.9.2 branch

So this means: No fix for Firefox 4?

Comment 15

[LegNeato]

(In reply to comment #14)
> > The regressing bug was backed out of the 1.9.2 branch
> 
> So this means: No fix for Firefox 4?

No. It is saying we were going to take a different fix in 3.6.13 but noticed that it caused this bug. Rather than inflicting this bug on all 3.6 users, we backed out the other fix.

This will likely be worked on for FF4, as the original patch that caused this issue is still in the 4.0 repository (I believe).

Comment 16

[Neil Deakin]

Attachment: Clear mRoot after calling Uninit

Comment 17

[Neil Deakin]

http://hg.mozilla.org/mozilla-central/rev/d87be167b410

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Manual, could you please verify that this bug is now fixed for you? Thanks.

Comment 19

[Sascha Grage]

yes, bug is fixed in firefox trunk build 20101213.

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Thanks Sascha! Marking as verified fixed based on comment 19.

Comment 21

[Neil Deakin]

Attachment: 1.9.2 patch

Comment 22

[LegNeato]

Comment on attachment 504810 [details] [diff] [review]
1.9.2 patch

a=LegNeato for 1.9.2.14

Comment 23

[Daniel Veditz [:dveditz]]

on Jan 18:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/34468ee8fdf2

Comment 24

[Daniel Veditz [:dveditz]]

Patch applied and seemed to fix bug on 1.9.1 branch, checked in (minus test)
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/bc91b067f42f

Comment 25

[Al Billings [:abillings - ex-MoCo]]

(In reply to comment #22)
> Comment on attachment 504810 [details] [diff] [review]
> 1.9.2 patch
> 
> a=LegNeato for 1.9.2.14

I don't see this in 1.9.2.13 with prefbar. I read above that we backed out the bug that caused this regression in 1.9.2. Does this bug reproduce on a released build?

Comment 26

[LegNeato]

No, it only happens when bug 596232 has landed, which will be in the next version and isn't in a released version. This would be a regression if we shipped bug 596232 alone w/o fixing this.

Comment 27

[Al Billings [:abillings - ex-MoCo]]

I'll mark it verified as I'm not seeing the bug in nightly builds then.