646767 - Tab Mix Plus add-on is not compatible with add-ons developed with Jetpack

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect)

Description

[Yixi]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; rv:2.0) Gecko/20100101 Firefox/4.0
Build Identifier: 

Tab mix plus add-on is not compatible with add-ons developed with Jetpack(https://jetpack.mozillalabs.com/) when 'Restore session" option in tab mix plus is enabled.

Reproducible: Always

Steps to Reproduce:
1) Install tab mix plus add-on https://addons.mozilla.org/en-US/firefox/addon/tab-mix-plus/
2) Enable the Resotore session Optinion (Check the screenshot below )
3) Install some other add-ons developed with Jetpack,such as Buzz This(https://addons.mozilla.org/en-US/firefox/addon/buzz-this/), Read Later Fast(https://addons.mozilla.org/en-US/firefox/addon/read-later-fast/)
4) Restart the browser. All add-ons developed with Jetpack disappear.

Actual Results:  
 All add-ons developed with Jetpack disappear.

Comment 1

[Brian Warner [:warner :bwarner]]

[edited title a bit]

Comment 2

[Alexandre Poirot [:ochameau]]

I tried, without being able to reproduce it.
But I looked at "read later fast" addon and its code is quite big and hacky!

Comment 3

[Yixi]

Attachment: tab mix plus options

Comment 4

[Yixi]

:Alex Please set the tab mix plus option as the screenshot in the attachment. 
(In reply to comment #2)
> I tried, without being able to reproduce it.
> But I looked at "read later fast" addon and its code is quite big and hacky!

Comment 5

[Yixi]

(In reply to comment #2) Add-ons developed with Jetpack must contain core libraries of Jetpack, so it's larger than many other extensions. 
 
> I tried, without being able to reproduce it.
> But I looked at "read later fast" addon and its code is quite big and hacky!

Comment 6

[Alexandre Poirot [:ochameau]]

Ok thanks for the screenshot, I didn't set this setting. I only accepted the popup at tabmixplus install that speak about session restore.
Now I'm able to reproduce.

I'll look at this tomorrow!

Comment 7

[Alexandre Poirot [:ochameau]]

Attachment: User browser-delayed-startup-finished event

First here is the "bug tracker item" in TabMixPlus community:
  http://tmp.garyr.net/forum/viewtopic.php?p=48307
Onemen already included a fix in his dev builds by dispatching "sessionstore-windows-restored" event from his extension.

I think that session store event should be dispatch in every cases, but when I read its code, I'm not completely sure of that:
http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/nsSessionStore.js#743

So even if I think that TabMixPlus have to send this event has it is used in others places like TestPilot or other firefox internals stuff. I would prefer using a better event that has to be dispatch in absolutely every cases without any doubt.
So I propose to use browser-delayed-startup-finished:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#1674
This event is dispatched by javascript code at the very end of the browser window setup:
- it's one of the very last events we can use, so it will ease firefox to launch more quickly. But it may change some existing firefox behavior (mainly extension that register listeners/obsrever)
- we can rely on this event because the JS function that dispatch it has to be called in order to browser to work correctly

Comment 8

[Yixi]

tab mix plus has fix this bug.
Anyway, thank you

Comment 9

[Yixi]

http://tmp.garyr.net/forum/viewtopic.php?t=10888

Comment 10

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

Ehsan: over in bug 529922, you added a browser-delayed-startup-finished notification.  onemen, one of the developers of Tab Mix Plus, suggests that SDK-based addons load on that notification rather than on sessionstore-windows-restored, which is when they currently load <http://groups.google.com/group/mozilla-labs-jetpack/browse_thread/thread/62b005a8400d010b/9b012d6b76f43272>.

But browser-delayed-startup-finished is not in the list of observer notifications <https://developer.mozilla.org/en/Observer_Notifications>, and it appears to have been added just to make a test work consistently, so it isn't clear that it's the right (stable, robust, tested, supported) notification for the SDK.

And addons that implement their own session manager, like Tab Mix Plus, seem pretty rare.  In fact, Tab Mix Plus is the only one I know about.  And it seems reasonable to require such addons to issue sessionstore-windows-restored (which Tab Mix Plus now does).

So I'm inclined to mark this bug fixed, now that Tab Mix Plus has been updated, and not make any further changes.  But I'd appreciate your opinion on the use of sessionstore-windows-restored versus browser-delayed-startup-finished.  What are your thoughts?

Comment 11

[(no longer active)]

So, I originally added browser-delayed-startup-finished for the sake of a test (and for only that reason).  This is an undocumented API, and it's possible that we might break/remove it in the future.

That said, I don't see why we might want to take it out.  Now that it's useful to an add-on, in addition to our test code, I'd say we document it in MDN.  CCing Gavin to see what he thinks about that.

But I don't think that there is any good reason why the SDK should use this notification instead of sessionstore-windows-restored.  So I think the SDK is better off continuing to use sessionstore-windows-restored, as it's the proper notification to see when the startup process has finished (whatever that means!).

Comment 12

[Alexandre Poirot [:ochameau]]

FYI: this event is already used by Firefox codebase:
http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#173

And I'd like to highlight that my main reason to push using this event is to use the latest possible event in order to increase firefox startup.

Actually, a lot of stuff is initialized before this event:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#1465

Comment 13

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

Comment on attachment 524227 [details] [diff] [review]
User browser-delayed-startup-finished event

Per Ehsan in comment 11, let's continue using the existing notification.

Comment 14

[(no longer active)]

(In reply to comment #12)
> FYI: this event is already used by Firefox codebase:
> http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#173

I added that code in bug 568816.  Read the discussion in that bug to see what sorts of stuff sucks in that patch!  ;-)

> And I'd like to highlight that my main reason to push using this event is to
> use the latest possible event in order to increase firefox startup.
> 
> Actually, a lot of stuff is initialized before this event:
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#1465

This is not really my call, I'll let Gavin correct me if I've been wrong.

Comment 15

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

We're not going to remove browser-delayed-startup-finished (in fact it will get another user in bug 434494). I can't really comment on whether it is suitable for use here, because I'm not very familiar with what the Jetpack addons expect. The choice seems somewhat arbitrary... I think "sessionstore-windows-restored" is most likely to fire later, but I'm not even sure that's true in the common case.