960864 - Placeholder text disappears in SearchBar after customize

Status: VERIFIED FIXED

(Firefox :: Search, defect)

Description

[Alice0775 White]

This problem is reproduced in Firefox27b6 and Aurora28.0a2, Holly29.0a1 and Nightly29.0a1.

Steps To Reproduce:
1. Start Firefox with newly created profile
2. Enter Customize toolbar mode
3. Exit Customize toolbar mode

Actual Results:
Placeholder text disappears in SearchBar

Comment 1

[Alice0775 White]

Regression window(fx-team)
Good:
http://hg.mozilla.org/integration/fx-team/rev/18acca8fc681
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20140109080042
Bad:
http://hg.mozilla.org/integration/fx-team/rev/1c7b312a2a85
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20140109093219
http://hg.mozilla.org/integration/fx-team/rev/113e065a2803
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20140109100819
Pushlog:
http://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=18acca8fc681&tochange=1c7b312a2a85

	f3e7fa7d9748	Gavin Sharp — Bug 903274: have the search bar binding's initialization callback bail out if the binding is destroyed, r=MattN

Comment 2

[Alice0775 White]

Firefox24.2pre also regressed due to Bug 903274 on Jan/11/2014.
http://hg.mozilla.org/releases/mozilla-esr24/rev/0a9219e71d17
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20140116 Firefox/24.0 ID:20140116000501

Comment 3

[Dão Gottwald [:dao]]

Attachment: patch

This makes _destroyed the exact opposite of _addedObserver. I started unifying them, but ended up removing the latter, since bug 722332 made it so that the observer is added directly in the constructor rather than asynchronously.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

We should just back out bug 903274 on early branches, and only take this followup on trunk.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

(In reply to Dão Gottwald [:dao] from comment #3)
> since bug 722332 made it so that the observer is added directly in the constructor
> rather than asynchronously.

(https://hg.mozilla.org/mozilla-central/rev/9f3af99c8297 for the record)

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Bug 903274 backed out from the release branches.

Comment 7

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/fx-team/rev/4d2cefab5e0f

Comment 8

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #7)
> https://hg.mozilla.org/integration/fx-team/rev/4d2cefab5e0f

This broke all the mochitest-browser-chrome tests that open customize mode, on all platforms, because the constructor's addObserver doesn't get run in customize mode, and your change means the destructor unconditionally tries to call removeObserver, which throws NS_ERROR_FAILURE in this case. Maybe:

-        var os = Components.classes["@mozilla.org/observer-service;1"]
-                           .getService(Components.interfaces.nsIObserverService);
-        os.removeObserver(this, "browser-search-engine-modified");
+        if (this.parentNode.parentNode.localName != "toolbarpaletteitem")
+          var os = Components.classes["@mozilla.org/observer-service;1"]
+                             .getService(Components.interfaces.nsIObserverService);
+          os.removeObserver(this, "browser-search-engine-modified");
+        }


? Or would you prefer to back out?

(that seems to fix it locally - but locally I'm also seeing leaks, which aren't fixed by that change. I don't know why that is.)

Comment 9

[:Gijs (he/him)]

In particular: https://tbpl.mozilla.org/?tree=Fx-Team&rev=f8b4174ffc65 with e.g. https://tbpl.mozilla.org/php/getParsedLog.php?id=33228928 as an example log.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/059ed9908fda

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Is there a better way in Australis for a node to know whether it's being customized rather than actually used? That's what that parentNode check was intended to do. At the very least we should use a more descriptive getter in the searchbar binding rather than calling getAttribute in both the destructor and constructor.

Comment 12

[u428464]

I don't know if it's related but the search bar is currently broken in customize mode without any text appearing in the palette and with the image half cut.

Comment 13

[:Gijs (he/him)]

(In reply to :Gavin Sharp (email [e-mail]) from comment #11)
> Is there a better way in Australis for a node to know whether it's being
> customized rather than actually used? That's what that parentNode check was
> intended to do. At the very least we should use a more descriptive getter in
> the searchbar binding rather than calling getAttribute in both the
> destructor and constructor.

document.documentElement.getAttribute("customizing") == "true"

is probably the easiest, assuming that's accessible in the destructor. If we're guaranteed to be in a browser window (is this binding used anywhere else?) then (window.)  CustomizationHandler.isCustomizing() will also give you what you want.

Comment 14

[Dão Gottwald [:dao]]

Attachment: patch v2

https://tbpl.mozilla.org/?tree=Try&rev=f17a7590bce3

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8363706 [details] [diff] [review]
patch v2

thanks

Comment 16

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/fx-team/rev/1061661a1119

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1061661a1119

Comment 19

[Cornel Ionce [:noni] [Hubs QA]]

Verified as fixed on FF 27 beta 9 (build ID: 20140123185438).

Comment 21

[Ioana (away)]

Verified as fixed with the steps from comment 0 (steps I previously reproduced the bug with) on the latest Firefox 28.0a2, Firefox 29.0a1 and Firefox 24.2.0esrpre (January 26th and 27th builds), on Windows 7 64bit, Mac OS X 10.7.5 and Ubuntu 13.04 32bit.