Closed
Bug 577349
Opened 14 years ago
Closed 14 years ago
Accessing gBrowser too early breaks other overlays
Categories
(Firefox :: Sync, defect)
Tracking
()
VERIFIED
FIXED
1.5
People
(Reporter: jwkbugzilla, Assigned: philikon)
References
Details
(Keywords: regression, Whiteboard: [fixed-1.4.2] [Input])
Attachments
(1 file)
1.57 KB,
patch
|
mconnor
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•14 years ago
|
||
What if you only comment out one? Or add os!=Darwin to the first line?
Reporter | ||
Comment 2•14 years ago
|
||
I cannot do any testing - it was Markus' Mac, maybe he can check.
Comment 3•14 years ago
|
||
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?
Assignee | ||
Comment 5•14 years ago
|
||
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.
Assignee | ||
Comment 6•14 years ago
|
||
(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.
Comment 7•14 years ago
|
||
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?
Updated•14 years ago
|
Whiteboard: [Input]
Assignee | ||
Comment 8•14 years ago
|
||
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.
Reporter | ||
Comment 9•14 years ago
|
||
(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.
Comment 10•14 years ago
|
||
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.
Comment 11•14 years ago
|
||
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).
Assignee | ||
Comment 12•14 years ago
|
||
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)
Assignee | ||
Comment 13•14 years ago
|
||
Oh, and you need to be on a Mac.
Comment 14•14 years ago
|
||
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
Comment 15•14 years ago
|
||
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.
Comment 16•14 years ago
|
||
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.
Comment 17•14 years ago
|
||
This isn't a problem in 3.6... can we get a regression window narrowed down?
Keywords: regressionwindow-wanted
Comment 18•14 years ago
|
||
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
Updated•14 years ago
|
Keywords: regressionwindow-wanted
Assignee | ||
Comment 19•14 years ago
|
||
The only changeset in that range that touched browser.xul was the one for bug 347930.
Comment 20•14 years ago
|
||
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
Updated•14 years ago
|
Keywords: regression
Reporter | ||
Comment 21•14 years ago
|
||
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
Comment 22•14 years ago
|
||
Sounds like bug 554279.
Assignee | ||
Comment 23•14 years ago
|
||
(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?
Reporter | ||
Comment 24•14 years ago
|
||
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.
Comment 25•14 years ago
|
||
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?
Assignee | ||
Comment 26•14 years ago
|
||
(In reply to comment #25) > Wondering if there is a race condition here? Yes, it's definitely a race condition.
Assignee | ||
Comment 27•14 years ago
|
||
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).
Assignee | ||
Updated•14 years ago
|
Summary: Applying Weave overlays twice breaks other overlays → Accessing gBrowser too early breaks other overlays
Assignee | ||
Comment 28•14 years ago
|
||
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)
Updated•14 years ago
|
Attachment #457850 -
Flags: review?(mconnor) → review+
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
Comment 29•14 years ago
|
||
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
Comment 30•14 years ago
|
||
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!
Updated•14 years ago
|
Status: RESOLVED → VERIFIED
Assignee | ||
Comment 31•14 years ago
|
||
(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).
Comment 32•14 years ago
|
||
(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 ago → 14 years ago
Comment 33•14 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
Comment 34•14 years ago
|
||
Jono/Dave: is this same thing happening on Test Pilot?
Updated•13 years ago
|
Flags: blocking-fx-sync1.5?
Updated•6 years ago
|
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.
Description
•