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

RESOLVED FIXED in mozilla28

Status

()

Core
XUL
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: Wladimir Palant, Assigned: Gijs)

Tracking

(Blocks: 1 bug, {regression})

Trunk
mozilla28
x86
Windows 7
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 Macaw+, status2.0 .1-fixed)

Details

(Whiteboard: [fx4-rc-ridealong])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

7 years ago
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.
(Reporter)

Comment 1

7 years ago
Created attachment 518042 [details]
Screenshot

Screenshot shows navigation toolbar with iconsize="large" after bringing up bookmark options - not pretty.
Can you reproduce this without adblock plus?
(Reporter)

Comment 3

7 years ago
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.
(Reporter)

Comment 4

7 years ago
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.
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.
Component: General → XUL
Product: Firefox → Core
QA Contact: general → xptoolkit.widgets
Whiteboard: regression
Created attachment 518047 [details] [diff] [review]
workaround (checked in)
Attachment #518047 - Flags: review?

Updated

7 years ago
Attachment #518047 - Flags: review? → review?(mak77)
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 :(
blocking2.0: ? → .x+
Whiteboard: [fx4-rc-ridealong]
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.
Attachment #518047 - Flags: review?(mak77) → review+
(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.
(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.

Updated

7 years ago
Attachment #518047 - Flags: approval2.0?
http://hg.mozilla.org/projects/cedar/rev/729114cbcda1
Whiteboard: [fx4-rc-ridealong] → [fx4-rc-ridealong] fixed-in-cedar

Comment 12

7 years ago
http://hg.mozilla.org/mozilla-central/rev/729114cbcda1
Assignee: nobody → dao
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fx4-rc-ridealong] fixed-in-cedar → [fx4-rc-ridealong]

Updated

7 years ago
Attachment #518047 - Attachment description: workaround → workaround (checked in)

Updated

7 years ago
Assignee: dao → nobody
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

7 years ago
Status: REOPENED → NEW

Comment 13

7 years ago
Why was this bug reopened?  Is the fix landed here not enough?
(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.

Updated

7 years ago
Duplicate of this bug: 645474
blocking2.0: .x+ → Macaw
Keywords: regression
Comment on attachment 518047 [details] [diff] [review]
workaround (checked in)

Approved for mozilla2.0 repository, a=dveditz for release-drivers
Attachment #518047 - Flags: approval2.0? → approval2.0+

Updated

7 years ago
Whiteboard: [fx4-rc-ridealong] → [fx4-rc-ridealong][not-ready-for-cedar]
(Reporter)

Comment 18

7 years ago
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

7 years ago
(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.)
(Reporter)

Comment 20

7 years ago
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.

Updated

7 years ago
Summary: Clicking star button changes toolbar's icon size from "small" to "large" → document.loadOverlay() reapplies localstore.rdf data (e.g. clicking star button changes toolbar's icon size from "small" to "large")
I think this one should block next release, if the problem can be activated by any add-on using loadOverlay in the browser window.
blocking-fx: --- → ?

Updated

7 years ago
Duplicate of this bug: 649203

Updated

7 years ago
blocking-fx: ? → ---

Updated

7 years ago
Duplicate of this bug: 603613

Updated

7 years ago
Summary: document.loadOverlay() reapplies localstore.rdf data (e.g. clicking star button changes toolbar's icon size from "small" to "large") → document.loadOverlay() reapplies localstore.rdf data causing deformed toolbar buttons (icon size changes from "small" to "large")

Updated

7 years ago
Summary: document.loadOverlay() reapplies localstore.rdf data causing deformed toolbar buttons (icon size changes from "small" to "large") → document.loadOverlay() reapplies localstore.rdf data (icon size changes from "small" to "large", causing deformed toolbar buttons)

Comment 24

7 years ago
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?

Updated

7 years ago
Duplicate of this bug: 655695

Updated

7 years ago
Duplicate of this bug: 665666

Comment 27

7 years ago
In which version of Firefox will this patch land?
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

7 years ago
Ok, how do I get this workaround? Im on the beta channel and still having this issue.
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

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

Updated

7 years ago
Whiteboard: [fx4-rc-ridealong][not-ready-for-cedar] → [fx4-rc-ridealong]

Updated

7 years ago
Duplicate of this bug: 681143

Comment 33

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

Updated

6 years ago
Blocks: 737158

Updated

6 years ago
Depends on: 747310

Updated

6 years ago
Blocks: 747310
No longer depends on: 747310

Comment 34

5 years ago
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).
Blocks: 943785
Created attachment 8342436 [details] [diff] [review]
, 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
Attachment #8342436 - Flags: review?(bzbarsky)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Created attachment 8342437 [details] [diff] [review]
restrict restoring persisted attributes when loading overlays after the first document load,

I swear bzexport should warn before uploading empty try push patches.
Attachment #8342437 - Flags: review?(bzbarsky)
Attachment #8342436 - Attachment is obsolete: true
Attachment #8342436 - Flags: review?(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
Attachment #8342437 - Flags: review?(bzbarsky) → review+
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
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...
Flags: needinfo?(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.
Flags: needinfo?(dao)
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.
Attachment #8342437 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/8853618ddaa8
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.