1014254 - Back button becomes disabled (and back/forward functionality breaks), after activating a share provider & customizing Share button away

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: My reduced prefs.js file that triggers this

I don't know exactly what got me into this state, but this afternoon I noticed that my "forward" button was never showing up. Still happened with safe mode. 

When I hacked away at my prefs.js file, I ended up in a state where back/forward functionality became completely broken.

I'm attaching the reduced prefs.js file, which just contains 3 entries:
 browser.uiCustomization.state
 social.activeProviders
 social.manifest.getpocket-com
which I've left at their values from my normal browsing profile. (Unlike in bug 1014238, I haven't intentionally messed with any of these prefs.)

STR:
 1. Run firefox (nightly) with a fresh profile, using the attached prefs.js file.
 2. Visit mozilla.org, and click through some links, to establish some history.
 3. Try to go back.
 4. Try to enter Customize mode. (e.g. by right-clicking toolbar and picking "Customize")

ACTUAL RESULTS:
 - Back button is non-functional (grayed out). If you right-click the page, the "Back"/"Forward" context-menu entries are also disabled.
 - Customize mode renders as a blank page.


NOTE: The symptoms here are slightly different from those in bug 1014238. In particular, the hamburger menu works here, and the "customize" toolbar-contextmenu-option does have an effect here, though it just ends up opening a blank page.

Comment 1

[Daniel Holbert [:dholbert]]

(It's possible that this is the same underlying issue as bug 1014238, but I've filed it separately for now, since the symptoms are slightly different, and since I actually ran into this with my real browsing profile whereas I'm kind of cheating in triggering bug 1014238.)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: screenshot of error console

Here's a screenshot of the error console, after I start Firefox with this prefs.js file.

Comment 3

[:Gijs (he/him)]

Per screenshot, this looks like it's related to the recent share button stuff. That's not good, because AIUI we were considering uplifting that...

Shane, got ideas here?

Comment 4

[Daniel Holbert [:dholbert]]

Most-relevant-looking parts of the error console, from that screenshot:
{
TypeError: shareButton is null
SocialShare.update@chrome://browser/content/browser.js:5600:34
SocialUI._providersChanged@chrome://browser/content/browser.js:5222:5
SocialUI_observe@chrome://browser/content/browser.js:5185:11
this.Social._updateProviderCache@resource://app/modules/Social.jsm:146:5
Social_init/<@resource://app/modules/Social.jsm:97:9
SocialServiceInternal.orderedProviders/<@resource://gre/modules/SocialService.jsm:82:9

  browser.js:5211

shareButton is null

  browser.js:5600
}

Comment 5

[Daniel Holbert [:dholbert]]

First error is linking to this "real" line of source:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-social.js?rev=324ce72cc5e3#167

Second error is linking to this "real" line of source:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-social.js?rev=324ce72cc5e3#556

Comment 6

[Daniel Holbert [:dholbert]]

For completeness, I'm testing with today's nightly:
 Mozilla/5.0 (X11; Linux x86_64; rv:32.0) Gecko/20100101 Firefox/32.0
 32.0a1 (2014-05-21)
 Built from https://hg.mozilla.org/mozilla-central/rev/9d8d16695f6a

Comment 7

[Daniel Holbert [:dholbert]]

In my normal browsing profile, I get the second error (for browser.js:5600, real url at the end of comment 5) every time I click a link or type in a URL to navigate to a new page.

Comment 8

[Daniel Holbert [:dholbert]]

Also, if I disable the Pocket social plugin [which fixes the bug] and then re-enable it [which brings the bug back], this appears in my error console:
{
docShell.QueryInterface(...).sessionHistory is null

  content-sessionStore.js:228
}
...which points to this line of source:
http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/content/content-sessionStore.js?rev=a7e3d4484692#228

(I'm only mentioning that since "sessionHistory" seems back/forward-navigation-related, and hence maybe part of this bug.)

Comment 9

[Daniel Holbert [:dholbert]]

(The disable/enable dance in comment 8 also makes the errors from comment 4 appear as well, right after the "sessionHistory is null" error-console-entry.)

Comment 10

[Daniel Holbert [:dholbert]]

I can repro in a 32-bit Nightly on Windows, too, FWIW. Exact same ACTUAL RESULTS as in comment 0.
 Mozilla/5.0 (Windows NT 6.3; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0
 32.0a1 (2014-05-21)

I believe mixedpuppy just repro'd on Mac, too. --> Setting Platform to All/All

Comment 11

[Shane Caraveo (:mixedpuppy)]

ok, full STR is:

- start fx with clean profile
- activate a share provider (e.g. facebook) from activations.cdn.mozilla.net
- customize the share button into the palette
- restart firefox

expecting: 
- back/forward buttons work
- share button is visible in the customization palette

current results:
- back/foward buttons are broken
- cannot enter customization

Once fixing the above, the share button does appear in the palette, but without an icon.  This is because the default state of the button is hidden.  Verifying this fix should verify that the icon is visible if the share button is in the palette at startup.

The fix I'll do for this right now will intentionally only get past the broken issues explained above.  More changes in the handling of the button will land with bug 1014332

Comment 12

[Shane Caraveo (:mixedpuppy)]

Attachment: fix button state when share is in customization palette

I haven't had time to look into new tests on this, but this patch fixes the problems, and current tests pass locally.  I'm actually not entirely certain how to write the test since this is a startup w/share in palette issue, though I just didn't get the time to think it through either.

Comment 13

[Shane Caraveo (:mixedpuppy)]

Attachment: fix button state when share is in customization palette

How about I actually attach a good patch file.

Comment 14

[:Gijs (he/him)]

Comment on attachment 8426821 [details] [diff] [review]
fix button state when share is in customization palette

Hrm. Why aren't we just updating the button when it's in the palette? That would also take care of the hidden state... we'd just need to adjust the shareButton getter to use CUI instead of doc.getElementById...

Comment 15

[:Gijs (he/him)]

The "make share button customizable" stuff is heading off to aurora with 32. I don't think we should ship it with this bug still present, so requesting tracking for 32 so we don't forget.

Comment 16

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gijs Kruitbosch from comment #14)
> Comment on attachment 8426821 [details] [diff] [review]
> fix button state when share is in customization palette
> 
> Hrm. Why aren't we just updating the button when it's in the palette? That
> would also take care of the hidden state... we'd just need to adjust the
> shareButton getter to use CUI instead of doc.getElementById...

I'm not clear on what you're saying here.  The getter does use CUI.

Comment 17

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gijs Kruitbosch from comment #15)
> The "make share button customizable" stuff is heading off to aurora with 32.
> I don't think we should ship it with this bug still present, so requesting
> tracking for 32 so we don't forget.

I looked at doing this a couple different ways, ultimately I think the right place to spend time is to refactor this into a full widget.  That cannot be done and uplifted since it changes the strings (from dtd to properties).  I'd like to move forward with this patch for 32 and deal with that refactoring on nightly later.

Comment 18

[:Gijs (he/him)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #16)
> (In reply to :Gijs Kruitbosch from comment #14)
> > Comment on attachment 8426821 [details] [diff] [review]
> > fix button state when share is in customization palette
> > 
> > Hrm. Why aren't we just updating the button when it's in the palette? That
> > would also take care of the hidden state... we'd just need to adjust the
> > shareButton getter to use CUI instead of doc.getElementById...
> 
> I'm not clear on what you're saying here.  The getter does use CUI.

Huh. So it does. Then the question is why it's null - it's a XUL widget, if it's in the palette we should be able to find the node.

Comment 19

[:Gijs (he/him)]

Oh. Right. So the shareButton getter bails out if !widget.areaType. Why does it do that?

Comment 20

[:Gijs (he/him)]

(FWIW, I think this is broken enough that we should still fix it for 32, although I can live with non-ideal fixes rather than, as you suggest, rewriting the widget to be a 'new style' CUI widget)

Comment 21

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gijs Kruitbosch from comment #19)
> Oh. Right. So the shareButton getter bails out if !widget.areaType. Why does
> it do that?

I was purposely not allowing code to act on the button if it is in the palette.  IMO it doesn't make sense to hide/show/disable the button in the palette.  Even removing that though, I still should to protect against the first null return.

rewriting is a bigger change, so lets get this into 32 and schedule the rewrite (bug 1030317)

Comment 22

[Shane Caraveo (:mixedpuppy)]

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

Comment 23

[:Gijs (he/him)]

Comment on attachment 8426821 [details] [diff] [review]
fix button state when share is in customization palette

Review of attachment 8426821 [details] [diff] [review]:
-----------------------------------------------------------------

WFM. Sorry for the delay here.

Comment 24

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/integration/fx-team/rev/920154597e3b

Comment 25

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/920154597e3b

Comment 26

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8426821 [details] [diff] [review]
fix button state when share is in customization palette

Approval Request Comment
[Feature/regressing bug #]:1003523
[User impact if declined]: Moving share button into customization palette breaks further access to palette as well as back/forward buttons
[Describe test coverage new/current, TBPL]: no new tests, manual testing is currently necessary
[Risks and why]: low
[String/UUID change made/needed]: none

Comment 27

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8426821 [details] [diff] [review]
fix button state when share is in customization palette

Aurora approval granted.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cc0ab48cb264

Comment 29

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 33.0a1 (2014-07-21) Win 7 x64.

Comment 30

[Petruta Horea [:phorea]]

Reproduced using Nightly 32.0a1 2014-05-22.
Verified as fixed using Firefox 32.0b1 under Win 7 64-bit, Ubuntu 12.10 32-bit, Mac OSX 10.9.4.