Closed Bug 577349 Opened 14 years ago Closed 14 years ago

Accessing gBrowser too early breaks other overlays

Categories

(Firefox :: Sync, defect)

x86
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: jwkbugzilla, Assigned: philikon)

References

Details

(Keywords: regression, Whiteboard: [fixed-1.4.2] [Input])

Attachments

(1 file)

I found out that the Adblock Plus toolbar button won't show up on OS X if Firefox Sync is installed (version 1.4 or 1.4.1b2). It is missing from the toolbar palette entirely - apparently, the Adblock Plus overlay applies incompletely. Looking at Sync's chrome manifest I see:

> overlay chrome://browser/content/browser.xul chrome://weave/content/firefox/sync.xul application={ec8030f7-c20a-464f-9b0e-13a3a9e97384} appversion<=4.0b2pre
> overlay chrome://browser/content/macBrowserOverlay.xul chrome://weave/content/firefox/sync.xul application={ec8030f7-c20a-464f-9b0e-13a3a9e97384} os=Darwin appversion<=4.0b2pre
> overlay chrome://browser/content/browser.xul chrome://weave/content/firefox/overlay.xul application={ec8030f7-c20a-464f-9b0e-13a3a9e97384}  appversion<=4.0b2pre
> overlay chrome://browser/content/macBrowserOverlay.xul chrome://weave/content/firefox/overlay.xul application={ec8030f7-c20a-464f-9b0e-13a3a9e97384} os=Darwin appversion<=4.0b2pre

Given that macBrowserOverlay.xul itself is applied to browser.xul this results in having these two Sync overlays being applied to browser.xul twice. I'm not sure why but this appears to be the reason for the breakage - commenting out the macBrowserOverlay lines "fixes" the issue.
What if you only comment out one?

Or add os!=Darwin to the first line?
I cannot do any testing - it was Markus' Mac, maybe he can check.
This bug isn't present on Windows.  

Requesting blocking sync1.5. We can't have sync hiding other extensions UI when it lands as a feature.
Severity: normal → critical
Flags: blocking-fx-sync1.5?
I can't reproduce this at all. I use Sync 1.4 and Adblock Plus in my personal profile and the button shows up. It's also there if I set up a brand new profile.

The Adblock Plus toolbar button is not there on nightlies, but that's independent of whether Sync is installed or not.
(In reply to comment #5)
> The Adblock Plus toolbar button is not there on nightlies, but that's
> independent of whether Sync is installed or not.

Actually, on a fresh profile the Adblock Plus toolbar button is always there now. Weird. Either way, it's still independent of whether Sync is installed or not.
This was also reported with the Feedback add-on on trunk.  Not sure why this would be an issue now, but we shouldn't need to overlay twice.  The browser.xul overlays should be os!=Darwin anyway, but why did this break now?
Whiteboard: [Input]
Adding os!=Darwin flags to the browser.xul overlay registrations doesn't help
at all. What does help is removing the macBrowserOverlay.xul overlay
registrations from chrome.manifest. Of course, that's not really a solution as
it removes the Sync submenu from non-browser windows (e.g. error console).

As the bug is not reproducible on Firefox 3.6 and other add-ons on 4.0b1 are
affected as well (see bug 578101), it seem that we're dealing with a trunk
regression here regarding chrome registration.
(In reply to comment #8)
> Adding os!=Darwin flags to the browser.xul overlay registrations doesn't help
> at all. What does help is removing the macBrowserOverlay.xul overlay
> registrations from chrome.manifest.

Does that mean that you could reproduce the issue with the Feedback add-on? Because if you interpreted my words this way - I didn't try removing the overlay on browser.xul or adding os!=Darwin.

And - yes, we were testing in a nightly build.
If we have problems with the overlay code on trunk, eventually bug 565610 comes into play here. Could be a side-effect. Please test a build from 2010-06-18 and 2010-06-19. I'm not aware of other changes for overlays on trunk, brought with the new add-ons manager.
I'm unable to reproduce this with Sync.  But I can with Tab Candy as the culprit.  That would be bug 577158.  That also breaks the Feedback button on build from 2010-06-18.  I don't know that the two extensions (TC and Sync) are actually caused by the same bug (though the end result is the same with disappearing feedback button).
To reproduce:

1. Start Minefield with fresh profile
2. Install Adblock Plus. Verify its toolbar button is shown.
3. Install Firefox Sync 1.4. Adblock Plus toolbar button now gone (even from palette)
Oh, and you need to be on a Mac.
I still can't repro with
 
build: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; en-US; rv:2.0b2pre) Gecko/20100714 Minefield/4.0b2pre 

Addons:
Adblock plus 1.2.1
weave-1.4.1b2-dev.xpi
Repro'd for me using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:2.0b1) Gecko/20100630 Firefox/4.0b1.

repro:
1) install minefield, and set Tabs on Top option.
2) install sync 1.4.1b2
3) apply a persona (any one works from getpersonas.com)
4) install adblock plus 1.2.1
5) feedback button gone.
OK, you don't need the Tab on Top setting nor the persona to reproduce this.  Just have feedback installed, install sync (you don't have to setup account), then install adblock plus.

I tested back to a nightly build of 20100402 (pre addons manager rewrite) and the bug is still present.
This isn't a problem in 3.6... can we get a regression window narrowed down?
Nightly mozilla-central one day window:

Works on 20100316 with: 
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a4pre) Gecko/20100316 Minefield/3.7a4pre
Built from http://hg.mozilla.org/mozilla-central/rev/050887c64183

Broken on 20100317 with:
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a4pre) Gecko/20100317 Minefield/3.7a4pre
Built from http://hg.mozilla.org/mozilla-central/rev/16a974dd72e1
The only changeset in that range that touched browser.xul was the one for bug 347930.
For reference:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=050887c64183&tochange=16a974dd72e1

(In reply to comment #19)
> The only changeset in that range that touched browser.xul was the one for bug
> 347930.

Dão Gottwald — Bug 347930 - Move the tabs outside the tabbrowser, put them in a toolbar. r=vlad
I doubt that this has anything to do with browser changes - it is rather something deeper that made overlay not apply properly. From what I can see, roc's layers only landed as an API and don't affect anything at this stage. Not too many other patches there that might have global side-effect, IMO only this one:

Alexander Surkov — Bug 352220  - Inconsistent focus events when returning to a document frame, r=marcoz, davidb
Sounds like bug 554279.
(In reply to comment #22)
> Sounds like bug 554279.

Yes, it does indeed sound like this is a dupe of bug 554279. According to Dão's explanation, the problem seems to arise when extensions trigger the toolbar constructors before XUL overlays for <toolbarpalette id="BrowserToolbarPalette"> are loaded. The toolbar constructors set toolbox.palette and remove the toolbarpalette element from the document so the overlays have no effect anymore and since their buttons are never added to toolbox. Accessing gNavToolbox (or gBrowser as gBrowser.tabContainer lives in a toolbar as of bug 347930) early enough apparently triggers this. Doing it in a "load" event handler or later should be fine, though.

Considering the behaviour reported in bug 578101 (a dupe of this bug) where BarTab, Sync and TestPilot (as integrated into trunk) clash, I can't seem to be able to fix the problem, though. TestPilot doesn't seem to access gBrowser at all, Sync does it in a "load" event handler, and BarTab in a "DOMContentLoaded" handler. So it seems BarTab is the culprit here, but changing the "DOMContentLoaded" handler to a "load" handler or even removing it altogether doesn't bring the Feedback button back.

Wladmir, does Adblock Plus access any toolbar or gBrowser programmatically on startup?
Philipp, Adblock Plus makes sure not to access anything before the window's "load" event - this is a well-known way to run into trouble. You can see the code under https://hg.adblockplus.org/adblockplus/file/ADBLOCK_PLUS_1_2_1_RELEASE/chrome/content/ui/overlay.js. But what you describe sounds like a logical explanation - I couldn't find the Adblock Plus button in the toolbar palette yet the Tools menu item which is defined in the same overlay was visible.
Just ran into this again. This time while updating from 1.4.1b2 to 1.4.1b3 with only Feedback 1.0rc1 and Sync installed.  

- Prior to the update with just the two features installed the Feedback button was present.
- Find Updates for Sync, Download, Restart Now.  Feedback button is gone.

Note: second attempt to do this by reverting to 1.4.1b2 then upgrading to b3 didn't show the problem.

Wondering if there is a race condition here?
(In reply to comment #25)
> Wondering if there is a race condition here?

Yes, it's definitely a race condition.
I modified the getters for the gBrowser, gNavToolbox, etc. globals in browser.js to dump the call stack on the first access. I also dump a message for the "load" and "DOMContentLoaded" events. Here's what happens on startup:

This is the hidden window on Mac (hiddenWindow.xul which specifies no windowtype="" attribute):

Window type '' fired event: DOMContentLoaded
Window type '' fired event: load
window[windowtype=].gBrowser accessed by Error()@:0
()@chrome://browser/content/browser.js:29
(17)@chrome://weave/content/firefox/overlay.js:49


Here we see that Sync is the culprit, specifically our code that tries to deduce how much to slow start up down by according to the number of open tabs:

window[windowtype=navigator:browser].gBrowser accessed by Error()@:0
()@chrome://browser/content/browser.js:29
onReady(null,null)@chrome://weave/content/firefox/sync.js:93
(null,"weave:service:ready",null)@resource://services-sync/ext/Observers.js:161
notifyObservers(null,"weave:service:ready",null)@:0
("weave:service:ready")@resource://services-sync/ext/Observers.js:122
([object Object])@resource://services-sync/service.js:306
notify([object XPCWrappedNative_NoHelper])@resource://services-sync/util.js:657

Window type 'navigator:browser' fired event: DOMContentLoaded
window[windowtype=navigator:browser].gURLBar accessed by Error()@:0
()@chrome://browser/content/browser.js:29
(null,null,4)@chrome://browser/content/browser.js:7280
()@chrome://browser/content/browser.js:6921
prepareForStartup()@chrome://browser/content/browser.js:4295
BrowserStartup()@chrome://browser/content/browser.js:4098
onload([object Event])@chrome://browser/content/browser.xul:1

window[windowtype=navigator:browser].gNavigatorBundle accessed by Error()@:0
()@chrome://browser/content/browser.js:29
("unknownIdentity")@chrome://browser/content/browser.js:9943
("unknownIdentity")@chrome://browser/content/browser.js:9871
(4,[object Object])@chrome://browser/content/browser.js:9839
(null,null,4)@chrome://browser/content/browser.js:7300
()@chrome://browser/content/browser.js:6921
prepareForStartup()@chrome://browser/content/browser.js:4295
BrowserStartup()@chrome://browser/content/browser.js:4098
onload([object Event])@chrome://browser/content/browser.xul:1

window[windowtype=navigator:browser].gNavToolbox accessed by Error()@:0
()@chrome://browser/content/browser.js:29
()@chrome://browser/content/browser.js:7652
()@chrome://browser/content/browser.js:7646
BrowserStartup()@chrome://browser/content/browser.js:4232
onload([object Event])@chrome://browser/content/browser.xul:1

Window type 'navigator:browser' fired event: load


I commented out the specific section of WeaveWindow.onReady() in ui/firefox/content/sync.js in which we access the gBrowser to get the list of tabs and the feedback button reappeared, horray! So basically we should look at having this code executed later (after "load" has fired in each navigator:browser window).
Summary: Applying Weave overlays twice breaks other overlays → Accessing gBrowser too early breaks other overlays
Attached patch v1Splinter Review
Bring back the "algorithm" that we had before the bug 570573 refactoring: Wait a bit before accessing window.gBrowser to do the sum over busy tabs. That way we avoid triggering the toolbar constructors too early.

Tested with both Adblock Plus (this bug) and Testpilot/Feedback (bug 578101).
Assignee: nobody → philipp
Attachment #457850 - Flags: review?(mconnor)
Attachment #457850 - Flags: review?(mconnor) → review+
http://hg.mozilla.org/services/fx-sync/rev/46db8014545c
Do not access gBrowser too early as this may break toolbar buttons provided by other overlays. 

http://hg.mozilla.org/services/fx-sync/rev/7f37de3e207d
1.4.x
Status: NEW → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [Input] → [fixed-1.4.2] [Input]
Target Milestone: --- → 1.5
Verified fix with Sync 1.4.2b1 Adblock plus 1.2.1, and Mozilla/5.0 (Windows; Windows NT 6.1; rv:2.0b2) Gecko/20100720 Firefox/4.0b2.  Feedback button persists!
Status: RESOLVED → VERIFIED
(In reply to comment #30)
> Verified fix with Sync 1.4.2b1 Adblock plus 1.2.1, and Mozilla/5.0 (Windows;
> Windows NT 6.1; rv:2.0b2) Gecko/20100720 Firefox/4.0b2.  Feedback button
> persists!

This issue occurred on OSX only (cf. comment 3).
(In reply to comment #31)
> This issue occurred on OSX only (cf. comment 3).

Correct. Tony, can you please re-verify? Thanks.
Status: VERIFIED → RESOLVED
Closed: 14 years ago14 years ago
thanks for catching.  i just copy/pasted the wrong build ID.  thats what happens when we have too many VM's open! 

Verified fix with Sync 1.4.2b1 Adblock plus 1.2.1,, and Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b3pre) Gecko/20100721 Minefield/4.0b3pre
Status: RESOLVED → VERIFIED
Jono/Dave: is this same thing happening on Test Pilot?
Flags: blocking-fx-sync1.5?
Component: Firefox Sync: UI → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: