635975 - Older jetpack addons cause tabs to become totally disorganized in Panorama

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P4)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

+++ This bug was initially created as a clone of Bug #628188 +++

I was bitten by bug 628188 when I installed https://addons.mozilla.org/en-US/firefox/addon/memory-meter/ as well.

Am I correct that the only solution here is to recompile any addons that used the old SDK with a newer SDK? Do we expect many jetpack addons to do this frequently? Is it likely that they will all do so by the Firefox 4 ship date?

If we're not confident that most addons will update themselves to a newer SDK I think we may need to figure out some workaround in the panorama code before we ship. Otherwise new Firefox 4 users will have have a really broken panorama, and that's one of our big new features.

What do you guys think?

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

Myk: at one point we had this idea of SDK updates being magically applied; is that still something that's a going concern?

(removing blocking nomination inherited from cloned bug)

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

The blocking nomination wasn't an artifact of the bug being cloned, I was actually interested in what the frontend folks think of the severity of this bug. 

I personally don't care enough about any of the currently available jetpack addons to really advocate for blocking here (I just removed the memory addon, problem solved).

Maybe others feel differently, though? It seems weird to promote jetpack and panorama in Firefox 4 and then have this big problem that arises when the two are mixed.

Comment 3

[Dietrich Ayala (:dietrich)]

Yeah, the problem is fixed in the newest SDK I think (not sure if it's released or not yet). 

About blocking, or wallpapering in Firefox:

1. The market should decide: if an add-on screws up your browser, it'll have crap reviews and ratings until the developer fixes it. People will not use it.

2. We are not going to change Firefox 4 to route around a few add-ons that are developed by people who are in our developer community (mostly Moco employees even!). Upgrading the SDK and recompiling takes minutes, so we should talk to those people.

3. Jetpack is still not 1.0. We try to minimize bugs and problems, but it's still not certified as being production quality. Problems like this bug are us being hurt by the success of it's immediate usefulness :)

4. These addons might be big in the mozilla developer community, but none of them are hitting critical mass. We need some perspective here. Of course, one post on RWW or Reddit could change that...

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

for the record. AddonSDK 1.0b3 that fixes this bug is only about to be released and is not yet available from inside the addons builder.

It'll be much easier to fix this once we have both things done :)

Comment 5

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #1)
> Myk: at one point we had this idea of SDK updates being magically applied; is
> that still something that's a going concern?

We're still considering automatically repackaging AMO-hosted addons with new minor security/stability releases of the SDK (once SDK 1.0 is released and we start doing such minor releases), although we haven't developed any firm plans to do so.

Comment 6

[Kevin Hanes]

(In reply to comment #3)
> 1. The market should decide: if an add-on screws up your browser, it'll have
> crap reviews and ratings until the developer fixes it. People will not use it.

The only caveat being that with this particular bug, it is not immediately clear that an add-on is the culprit. It looks like Panorama just falls over.

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Still don't think it blocks; earlier versions of the SDK were pre-alpha. Not expected to always work without recompiling.

Comment 8

[Dietrich Ayala (:dietrich)]

(In reply to comment #6)
> The only caveat being that with this particular bug, it is not immediately
> clear that an add-on is the culprit. It looks like Panorama just falls over.

!!! The discussion up until now has been entirely about add-ons causing the problem. Can someone on the Panorama team investigate this asap?

Comment 9

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #8)
> (In reply to comment #6)
> > The only caveat being that with this particular bug, it is not immediately
> > clear that an add-on is the culprit. It looks like Panorama just falls over.
> 
> !!! The discussion up until now has been entirely about add-ons causing the
> problem. Can someone on the Panorama team investigate this asap?

My understanding is that, while this *is* the fault of the jetpack API itself, to the user it *looks* like it's Panorama's epic fail. Kevin, can you clarify?

Comment 10

[Kevin Hanes]

Every time this bug has manifest itself, it has been filed as "Panorama randomly re-groups items". In it's original form, we did quite a bit of investigation (fruitlessly, as none of us had jetpack add-ons) until we noticed that everyone reporting this bug had the "hardblocker counter" add-on or some other jetpack add-on.

So while we know this is a jetpack bug, its negative side effects only manifest visibly in Panorama. It is not immediately clear (even to developers and other technical people, let alone someone without domain knowledge)that an add-on is causing this, thus it looks like Panorama is just falling down.

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Yeah, this is why I was thinking we might need a panorama hack.

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

Small fix based on the fact that "domwindowclosed" is fired before "unload".

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 516380 [details] [diff] [review]
patch v1

Passed try:

http://tbpl.mozilla.org/?tree=MozillaTry&pusher=[e-mail]&rev=7afcc955c3cf

Comment 14

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Ian, any chance you could review this quickly? It's small enough (and I'd say high enough reward) that we may be able to slip this in for 4.0 if we move fast.

Comment 15

[Ian Gilman [:iangilman]]

Comment on attachment 516380 [details] [diff] [review]
patch v1

Seems worth doing.

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 516380 [details] [diff] [review]
patch v1

Note to approvers:

This is a really small patch the prevents tabs from being unlinked when the dom windows is closing. It's a little workaround for a fixed jetpack bug that unfortunately requires all older jetpack add-ons to be updated.

Comment 17

[Ian Gilman [:iangilman]]

(In reply to comment #16)
> Comment on attachment 516380 [details] [diff] [review]
> patch v1
> 
> Note to approvers:
> 
> This is a really small patch the prevents tabs from being unlinked when the dom
> windows is closing. It's a little workaround for a fixed jetpack bug that
> unfortunately requires all older jetpack add-ons to be updated.

... but which looks to the user like a Panorama failure.

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 516380 [details] [diff] [review]
patch v1

Too late, if we do an RC2 spin we could consider it as a ridealong.

Comment 19

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 516380 [details] [diff] [review]
patch v1

A little bird told me we are respinning, can we please grab this one?

Comment 20

[Andreas Gal :gal]

Comment on attachment 516380 [details] [diff] [review]
patch v1

Not this round. Please land when the tree re-opens. And maybe contact evangelism?

Comment 21

[Kevin Hanes]

(In reply to comment #20)
> Comment on attachment 516380 [details] [diff] [review]
> patch v1
> 
> Not this round. Please land when the tree re-opens. And maybe contact
> evangelism?

That would be lovely, who/how should we contact?

Comment 22

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 516380 [details] [diff] [review]
patch v1

Let's get this in to 2.0.1 then.

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch for checkin

Comment 24

[Tim Taubert [:ttaubert] (inactive)]

We should land this on trunk.

Comment 25

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 521990 [details] [diff] [review]
patch for checkin

And we'd still want this in 2.0.1. :)

Comment 26

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/9ef10b9170bc

Comment 27

[Daniel Veditz [:dveditz]]

Comment on attachment 521990 [details] [diff] [review]
patch for checkin

Approved for the mozilla2.0 repository, a=dveditz for release-drivers

Comment 28

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-2.0/rev/062e9361345e

Comment 29

[George Carstoiu]

Verified with Mozilla/5.0 (Windows NT 5.1; rv:2.2a1pre) Gecko/20110410 Firefox/4.2a1pre