640158 - document.loadOverlay() reapplies localstore.rdf data (icon size changes from "small" to "large", causing deformed toolbar buttons)

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Wladimir Palant]

Bug 631491 appears to have created an upgrade issue for people who reuse their Firefox 3.x profile with Firefox 4. Steps to reproduce:

1. Run Firefox 3.x, customize toolbar and switch icon size once or twice to get large icons (makes sure iconsize="large" is persisted).
2. Now run Firefox 4 with the same profile.
3. Install Adblock Plus 1.3.3 from https://addons.mozilla.org/addon/adblock-plus/ and customize toolbar to move its icon from add-on bar into navigation toolbar (this merely makes the issue obvious). Note that you see a small ABP icon.
4. Now click star button (bookmark) twice to bring up bookmark options.

Expected results: icon size stays the same

Actual results: toolbar switches to large icons (iconsize attribute changes from "small" to "large") and Adblock Plus icon stretches all the other buttons in the toolbar.

This regressed between 2011-03-03 and 2011-03-04 Minefield builds, local backout confirmed that bug 631491 caused this regression (or at least made the issue obvious). Requesting blocking2.0 flag - I guess that lots of users will hit this when migrating from Firefox 3.6.

Comment 1

[Wladimir Palant]

Attachment: Screenshot

Screenshot shows navigation toolbar with iconsize="large" after bringing up bookmark options - not pretty.

Comment 2

[Dão Gottwald [:dao]]

Can you reproduce this without adblock plus?

Comment 3

[Wladimir Palant]

As I said - Adblock Plus only makes the issue visible. iconsize="large" is being set regardless of any extensions but without extensions there is no toolbar button that will respect this attribute.

Comment 4

[Wladimir Palant]

Just reproduced with a clean profile, only installed DOM Inspector to check attributes. After "upgrading" from Firefox 3.6 to Firefox 4 I see iconsize="small" on the nav-bar toolbar, bringing up bookmark options changes this attribute into iconsize="large". Without any custom buttons this doesn't have any visual effects of course.

Comment 5

[Dão Gottwald [:dao]]

Looks like document.loadOverlay("chrome://browser/content/places/editBookmarkOverlay.xul", ...) from browser-places.js causes localstore.rdf to be read and applied a second time on the entire document. Moving to core:xul.

Comment 6

[Dão Gottwald [:dao]]

Attachment: workaround (checked in)

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Ew.

I don't think this will cause a respin of the RC for the following reasons:

 - while visible, it requires some heavy customization which (presumably) can be undone to restore a good state
 - it requires non-standard configurations for displaying toolbar icons
 - we can fix it in a .x

Hurts me to not block, hurts more that it came from a non-blocking fix that we probably just shouldn't have approved :(

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 518047 [details] [diff] [review]
workaround (checked in)

The iconsize attribute is packing both a user choice and a theme override, wouldn't be healthier to have 2 separate attributes and put css rules in a proper order for override? At that point the theme override could still not be persisted, but it would not race with the user choice.

Apart this, if this is not going to be an emergency patch, it's better to figure out the localstore re-apply issue than taking a workaround.

If otherwise we go for the workaround in a RC respin, I think we should file a bug to proper fix the underlying issue, annotate in that bug that we should remove this workaround once fixed, and have a comment like:
// TODO (bug 123456): loading an overlay should not re-apply
//                    localstore.rdf to the whole document.
The r+ is conditioned to this case, if the respin won't happen we should directly patch the underlying issue.

Comment 9

[Dão Gottwald [:dao]]

(In reply to comment #8)
> The iconsize attribute is packing both a user choice and a theme override,
> wouldn't be healthier to have 2 separate attributes and put css rules in a
> proper order for override?

There are too many extensions using the iconsize attribute, so we don't want to change that unless absolutely necessary.

> Apart this, if this is not going to be an emergency patch, it's better to
> figure out the localstore re-apply issue than taking a workaround.

For the 2.0 branch I think we only want the workaround, regardless of whether it's going to land for 4.0 or a dot release.

> If otherwise we go for the workaround in a RC respin, I think we should file a
> bug to proper fix the underlying issue

I'm not going to close this bug.

Comment 10

[Marco Bonardo [:mak]]

(In reply to comment #9)
> > Apart this, if this is not going to be an emergency patch, it's better to
> > figure out the localstore re-apply issue than taking a workaround.
> 
> For the 2.0 branch I think we only want the workaround, regardless of whether
> it's going to land for 4.0 or a dot release.

yeah, fine for stable branch.

Comment 11

[Dão Gottwald [:dao]]

http://hg.mozilla.org/projects/cedar/rev/729114cbcda1

Comment 12

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/729114cbcda1

Comment 13

[(no longer active)]

Why was this bug reopened?  Is the fix landed here not enough?

Comment 14

[Dão Gottwald [:dao]]

(In reply to comment #13)
> Why was this bug reopened?  Is the fix landed here not enough?

It's a workaround, it doesn't solve the underlying problem.

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 518047 [details] [diff] [review]
workaround (checked in)

Approved for mozilla2.0 repository, a=dveditz for release-drivers

Comment 17

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-2.0/rev/cc6343052a96

Comment 18

[Wladimir Palant]

A user reported that this issue is happening on each browser restart for him. I guess that some extension is triggering it when the browser window opens.

Comment 19

[Jacob W. Klein]

(In reply to comment #18)
> A user reported that this issue is happening on each browser restart for him. I
> guess that some extension is triggering it when the browser window opens.

I am that user, and it appears that the retrigger-on-browser-restart issue happens with a combination of Adblock Plus 1.3.6 RC, and Test Pilot 1.1.

If I use Adblock Plus 1.3.6 RC without Test Pilot 1.1, and have the ABP button on the same row as the Back/Forward buttons, and do not "Use Small Icons", then on browser restart, the sizing of the row still looks good.

If I use Adblock Plus 1.3.6 RC WITH Test Pilot 1.1, and have the ABP button on the same row as the Back/Forward buttons, and do not "Use Small Icons", then on browser restart, the size of the row becomes too tall, and the Back/Forward buttons look too vertically stretched (since the whole row is vertically stretched.)

Comment 20

[Wladimir Palant]

I can confirm that Test Pilot 1.1 triggers this issue. It calls method TestPilotUIBuilder.buildCorrectInterface() when the browser window loads and that function then calls window.document.loadOverlay() to apply one of its two browser window overlays.

Comment 21

[Marco Bonardo [:mak]]

I think this one should block next release, if the problem can be activated by any add-on using loadOverlay in the browser window.

Comment 24

[Chris B.]

Where's the Macaw download?  I thought it was first supposed to be released on Tuesday then got pushed to today (Thursday).  Patches for this and bug 645551 are ready to land, but the Firefox 4 download page still has 4.0.0.  So, what's the big wait for TWO patches?

Comment 27

[heraldo]

In which version of Firefox will this patch land?

Comment 28

[Marco Bonardo [:mak]]

there is no patch for the underlying issue yet (otherwise it would be resolved fixed), the workaround is on all versions from 4.0 on.

Comment 29

[heraldo]

Ok, how do I get this workaround? Im on the beta channel and still having this issue.

Comment 30

[Marco Bonardo [:mak]]

you already have it, the workaround fixes a well known issue, but not all of them, otherwise this bug would be fixed. Some add-on is known to cause this bug still.

Comment 31

[Frederik Bovendeerd]

Why did I not see this before sending in my report?
Is bug 681143 a duplicate of the above discussed problem?

Comment 33

[Adrian]

(In reply to Wladimir Palant from comment #20)
> I can confirm that Test Pilot 1.1 triggers this issue. It calls method
> TestPilotUIBuilder.buildCorrectInterface() when the browser window loads and
> that function then calls window.document.loadOverlay() to apply one of its
> two browser window overlays.

I have disabled Test Pilot 1.2 in Mozilla Aurora 10 and can confirm this fixes my issue. Prior to this, even customizing the toolbar and resetting everything did nothing. Toggling small icons fixed the issue for a moment as opening a new window always brought the issue back. As for old profile regressions, my profile is a fresh one from Aurora 9. Everything is then drawn from my Weave account which overwrites my existing default profile.

I don't know why this is happening but of all my addons, disabling Test Pilot 1.2 is the only one that reliably stopped these large icons from coming back.

Comment 34

[Kris]

I confirm that, like Adrian in Comment 33, I faced the same "big module icon" issue while installing Firefox 20 beta i.e. TestPilot. Even when manually resized to small, the issue rehappened after a browser restart.

Disabling TesPilot module and restarting the browser immediately makes all module icons of the toolbar smaller (tested with AdBlockPlus and GreaseMonkey icons).

Comment 35

[:Gijs (he/him)]

Attachment: , try: -b od -p win32,macosx64,linux,linux64 -t none -u all

So... I am a stranger in content/xul land, but I believe this fixes the bug we're running into. Of course, we could just ignore persisted data after the first document load, but I figured that we do want content in overlays loaded through loadOverlay to have persisted data... so I implemented something slightly more complicated. The good news being that I did also write a test for it, and it seems to work. Try push:  https://tbpl.mozilla.org/?tree=Try&rev=3972b191dda4

Comment 36

[:Gijs (he/him)]

Attachment: restrict restoring persisted attributes when loading overlays after the first document load,

I swear bzexport should warn before uploading empty try push patches.

Comment 37

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8342437 [details] [diff] [review]
restrict restoring persisted attributes when loading overlays after the first document load,

Seems reasonable, but please brace single-line if bodies in this code.

r=me

Comment 38

[:Gijs (he/him)]

Comment on attachment 8342437 [details] [diff] [review]
restrict restoring persisted attributes when loading overlays after the first document load,

(In reply to Boris Zbarsky [:bz] from comment #37)
> Comment on attachment 8342437 [details] [diff] [review]
> restrict restoring persisted attributes when loading overlays after the
> first document load,
> 
> Seems reasonable, but please brace single-line if bodies in this code.
> 
> r=me

Landed with this nested one fixed:

>diff --git a/content/xul/document/src/XULDocument.cpp b/content/xul/document/src/XULDocument.cpp
>@@ -2971,16 +2982,24 @@ XULDocument::ResumeWalk()
>+                    // If we're only restoring persisted things on
>+                    // some elements, store the ID here to do that.
>+                    if (mRestrictPersistence) {
>+                        nsIAtom* id = child->GetID();
>+                        if (id)
>+                            mPersistenceIds.PutEntry(nsDependentAtomString(id));
>+                    }
>+


But not this one:

>@@ -2219,16 +2227,19 @@ XULDocument::ApplyPersistentAttributesIn
>             continue;
> 
>         nsAutoString id;
>         nsXULContentUtils::MakeElementID(this, nsDependentCString(uri), id);
> 
>         if (id.IsEmpty())
>             continue;
> 
>+        if (mRestrictPersistence && !mPersistenceIds.Contains(id))
>+            continue;
>+
>         // This will clear the array if there are no elements.
>         GetElementsForID(id, elements);
> 
>         if (!elements.Count())
>             continue;
> 
>         ApplyPersistentAttributesToElements(resource, elements);
>     }


Because all the other if statements in that block use the non-brace style. I'm *guessing* the top one was really what you meant; so in the spirit of asking for forgiveness rather than permission, I've landed as such. Please let me know if I was wrong. :-)

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8853618ddaa8

Comment 39

[:Gijs (he/him)]

Dão, the workaround has been removed in Australis because the persistent icon sizes are gone there. Assuming the general patch sticks and goes through to Holly, do you think it's worth backing out the workaround there?

And do you know, offhand, if we have other workarounds in place for this bug that need to be cleared up? A MXR search for this bug number yielded nothing, but that might not be all there is to this...

Comment 40

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #39)
> Dão, the workaround has been removed in Australis because the persistent
> icon sizes are gone there. Assuming the general patch sticks and goes
> through to Holly, do you think it's worth backing out the workaround there?

I don't think it's worth it, as the workaround shouldn't have a negative effect even if it's redundant.

> And do you know, offhand, if we have other workarounds in place for this bug
> that need to be cleared up? A MXR search for this bug number yielded
> nothing, but that might not be all there is to this...

I don't think there are other workarounds. If I had added one, I would likely have added a comment with this bug number.

Comment 41

[:Gijs (he/him)]

Comment on attachment 8342437 [details] [diff] [review]
restrict restoring persisted attributes when loading overlays after the first document load,

(In reply to Dão Gottwald [:dao] from comment #40)
> (In reply to :Gijs Kruitbosch from comment #39)
> > Dão, the workaround has been removed in Australis because the persistent
> > icon sizes are gone there. Assuming the general patch sticks and goes
> > through to Holly, do you think it's worth backing out the workaround there?
> 
> I don't think it's worth it, as the workaround shouldn't have a negative
> effect even if it's redundant.
> 
> > And do you know, offhand, if we have other workarounds in place for this bug
> > that need to be cleared up? A MXR search for this bug number yielded
> > nothing, but that might not be all there is to this...
> 
> I don't think there are other workarounds. If I had added one, I would
> likely have added a comment with this bug number.

Makes sense, thanks for the quick response.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8853618ddaa8