Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support onEnabled event when the extension from disabled state to enabled state #353

Open
hanguokai opened this issue Feb 16, 2023 · 32 comments
Labels
enhancement Enhancement or change to an existing feature supportive: chrome Supportive from Chrome supportive: firefox Supportive from Firefox supportive: safari Supportive from Safari

Comments

@hanguokai
Copy link
Member

References:

  • There were many discussions about the lifecycle of extension in this thread.
  • Issue 388231: chrome.runtime.onInstalled not run when extension is updated while its disabled

Problem Description

Extensions that are disabled are very common. You can see what percentage of users have your extension enabled and disabled from the extension store dashboard.

At present, there are some events when an extension startup, i.e. runtime.onInstalled and runtime.onStartup. But there is no event when an extension from disabled state to enabled state, for example, users disable then re-enable an extension.

Similar to the purpose of using onInstalled and onStartup event, sometimes onEnabled event is also needed.

Possible Solutions

1. New API: runtime.onEnabled

browser.runtime.onEnabled.addListener(() => { ... })

2. Fire an event in runtime.onInstalled

browser.runtime.onInstalled.addListener(details => {
    if (details.reason == "enable") {
      ……
    }
})

Here, add a new reason in runtime.OnInstalledReason.

3. Allow to use management.onEnabled without the "management" permission

browser.management.onEnabled.addListener(info => { ... })

At present, management.onEnabled is required "management" permission to use it, even though you just listen yourself.

@xeenon xeenon added supportive: safari Supportive from Safari and removed needs-triage labels Feb 16, 2023
@Rob--W Rob--W added follow-up: chrome Needs a response from a Chrome representative supportive: firefox Supportive from Firefox labels Feb 16, 2023
@Rob--W
Copy link
Member

Rob--W commented Feb 16, 2023

I'm supportive of supporting runtime.onInstalled with a new reason.

@tophf
Copy link

tophf commented Feb 21, 2023

1 and 3 can work, 2 definitely won't because runtime.onInstalled clearly and unequivocally says it's about installation but none happens on re-enabling.

@fastaddons
Copy link

From the practical point of view, I would like this to be a runtime.onInstalled event PLUS I would like to have also previousVersion field set to a correct value (the version of extension when it was disabled).

Since extensions are updated even when they are disabled, they are missing all runtime.onInstalled events with update reason. So this would allow to fix that issue.

@tophf
Copy link

tophf commented Feb 21, 2023

Having a misleading API is not really practical, so onInstalled should be about installations.

updated even when they are disabled, they are missing all runtime.onInstalled events

That's a good point. I guess the solution might be to dispatch the missing onInstalled either in addition to onEnabled or instead of it. In both cases the onInstalled event parameter should probably contain an additional property like wasDisabled: true or previouslyDisabled: true.

@hanguokai
Copy link
Member Author

event PLUS I would like to have also previousVersion field set to a correct value (the version of extension when it was disabled).

An edge case is that an extension is updated to a new version, then the browser find it has new warning permissions and disable it. The user may manually enable it later (for a short or long time). When it is enabled again, there should be only an "enable" event and no "update" event. In the "enable" event, developers usually execute some one time startup logic code and should not reply on the "previousVersion" (developers can check their own flags). So the "enable" event also means it may be updated.

@tophf
Copy link

tophf commented Mar 1, 2023

Quoting @oliverdunk in https://github.com/w3c/webextensions/blob/main/_minutes/2023-02-16-wecg.md

runtime.onInstalled is not fired in unpacked extensions in Chrome.

This is not true. The event is actually fired for an unpacked extension in Chrome when you reload it because reloading is the same as an update, which includes re-installation.

Naturally onInstalled won't be fired when the extension was disabled and then re-enabled without reinstalling because there was no installation and logically this should remain unchanged i.e. I disagree with Rob's idea because it's a violation of the good programming practice that a function does the thing implied by its name and doesn't do things outside the scope implied by its name.

@oliverdunk
Copy link
Member

Sorry, I think the minutes slightly missed what I was trying to say (which is totally fine, taking minutes is hard!). All I wanted to point out is that there are some interesting non-intuitive behaviours for unpacked extensions. One that comes to mind is that I don't think you can easily test the update reason.

@tophf
Copy link

tophf commented Mar 9, 2023

don't think you can easily test the update reason

You can simply click the reload icon. What's not trivial is to emulate automatic update behavior but that's beside the point here.

@oliverdunk oliverdunk removed the follow-up: chrome Needs a response from a Chrome representative label Mar 10, 2023
@oliverdunk
Copy link
Member

@tophf: I just tested and you're totally right. I'm not sure what behaviour I was thinking of but clearly it wasn't this.

On the larger proposal, I've followed up about this on the Chrome side. We're generally supportive of the idea, but don't feel super comfortable with using the existing event. We would prefer something that we can be sure won't break extensions making a false assumption based on the function name (admittedly this is already a problem with install vs. update for example, but it would be nice to avoid making it worse).

It might be worth having some more discussion here either in the next meeting or just when we next decide there's a browser that's looking to implement it.

@hanguokai
Copy link
Member Author

I proposed three solutions in my original post, all of which make sense to some extent. At present, there is no workaround for onEnabled event. So either solution is better than no solution.

About name problem, onInstalled combined multiple different reason events. And onStartup is not extension first startup, it is browser profile startup. Both names are misleading. Therefore, due to historical legacies, the misleading names are difficult to completely solve.

If the name is the last stumbling block for this feature, perhaps the option one is more appropriate.

@tophf
Copy link

tophf commented Mar 10, 2023

onStartup at least does not lie about it being a startup. Also, it's wrong to use an existing problem to justify adding more problems, instead we should probably mark onStartup as deprecated and add something like onProfileStartup or improve documentation for onStartup.

@dotproto dotproto added the enhancement Enhancement or change to an existing feature label Mar 15, 2023
@dotproto dotproto changed the title Proposal: support onEnabled event when the extension from disabled state to enabled state Support onEnabled event when the extension from disabled state to enabled state Mar 15, 2023
@dotproto
Copy link
Member

Tweaking the title as this is more of a feature request than a specific proposal.

@hanguokai
Copy link
Member Author

Tweaking the title as this is more of a feature request than a specific proposal.

This is already a simple and clear proposal. The functionality is clear and the implementation is not a problem. If browser vendors can reach an agreement, it can be done quickly.

@oliverdunk
Copy link
Member

Sharing some thoughts ahead of the meeting. From what I can tell there are two main use cases (I'm sure there are more, so please feel free to call those out):

  • Detecting when an extension that was disabled due to a permission change on update has been enabled.
  • Generally wanting to run code once in a given extension's "lifecycle", without storing a flag to keep track of this. This would include enabling the extension, as well as things like firing once when a particular browser profile is first started. It's basically the equivalent of runtime.onStartup but you would also get it for enabling an extension.

With those in mind, I wonder if something like a new runtime.onExtensionStartup would be most helpful, which fires on both profile startup and when the extension is enabled. Perhaps it could optionally have some sort of updatedFromVersion field in the event data.

The reason I'm wondering about this instead of onEnabled is that it solves both use cases whereas onEnabled only solves the first.

Also another thought - I could see an argument that not firing runtime.onInstalled for updates after an extension is enabled is a bug. Maybe we just fix that and we solve the first use case without anything new at all? This is basically the discussion in https://bugs.chromium.org/p/chromium/issues/detail?id=388231 which I realise hasn't moved for a while.

Thinking out loud here, haven't spoken to the rest of the Chrome team about this...

@hanguokai
Copy link
Member Author

hanguokai commented Mar 30, 2023

What is the definition of an extension "startup"? It's a bit confusing. Many people intuitively think that "startup" is the first execution in an extension life cycle. By this definition, "extension startup" should include:

  • browser(profile) launch/re-launch: when browser startup or update, a browser profile first(not second) startup.
  • extension onEnabled: disable then re-enable it manually or permissions changed when update.
  • extension install: Installation means its first startup.
  • extension update: Update means a new version of this extension first startup.

The above cases fully cover the first execution in an extension life cycle.

For multiple profiles browser like Chrome, assuming two profile windows are opened, will extensions be terminated when one profile window is closed? In my test, the answer is "No". A persistent MV2 extension is still alive when the profile window is closed. Alarm events(MV2 and MV3) still fired when the profile window is closed. So only when the profile first startup means extensions in that profile first startup. Therefore, browser profile startup does not necessarily mean extension startup.

@oliverdunk
Copy link
Member

@hanguokai I think all of those should count 🙂

A persistent MV2 extension is still alive when the profile window is closed.

Do you think there are cases where this distinction is important for the API to be useful? I'm leaning towards no, but we should still do our best to define it so it can be consistent between browsers.

@hanguokai
Copy link
Member Author

Do you think there are cases where this distinction is important for the API to be useful?

@oliverdunk According to the definition of extension startup (runtime.onExtensionStartup is triggered only on its first execution), if the extension has already started before, the extension startup event should not be triggered again when the profile window is subsequently opened. You know, in Chrome, the profile startup event (runtime.onStartup) is triggered every time when the profile window is opened. For example, Profile-A window is opened, now you open Profile-B window then close Profile-B window, and then open Profile-B window again and close Profile-B window again …… The extension startup event should only fired on the first time of the Profile-B window is opened. This is the difference between "extension startup" and "profile startup" in multi-profiles-supported browser.

If you want to introduce runtime.onExtensionStartup instead of runtime.onEnabled, it should include the cases I mentioned earlier and pay attention to the difference between "extension startup" and "profile startup".

@oliverdunk
Copy link
Member

@hanguokai / anyone else: I've been trying to reproduce the issue with onInstalled not firing for disabled extensions, but I haven't been able to reproduce it [1]. Are you hitting this, and do you have steps to reproduce, or is it possible that issue is fixed?

I'm not proposing to close this discussion but want to understand what works today.

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=388231#c13

@hanguokai
Copy link
Member Author

@oliverdunk Off topic, I suggest that browsers provide a mock method for developers to test upgrades from the extension store. This will solve a lot of debugging and testing problems, including the one you're facing. I'd love to help you, but I don't have an extension at hand that needs to be published.

@oliverdunk
Copy link
Member

@oliverdunk Off topic, I suggest that browsers provide a mock method for developers to test upgrades from the extension store. This will solve a lot of debugging and testing problems, including the one you're facing. I'd love to help you, but I don't have an extension at hand that needs to be published.

On the Chrome side (as I think you've seen) we recently released a testing tool which can be used for this: https://github.com/GoogleChromeLabs/extension-update-testing-tool

From the last meeting's minutes:

[Oliver] Would personally be interested in an event to transition from non-running to running, but there were reservations about it due to the questions about the usefulness of this.

I mentioned in the last meeting that I'm still optimistic about a runtime.onExtensionStartup event but wanted to put together a more complete list of use cases. I still need to have more conversations on the Chrome side but I have put together a list:

  • I want to open a WebSocket connection when the profile opens, the user enables my extension or the user installs my extension for the first time.
  • I want to register some DNR session rules.
  • I need to pre-load some data into storage.session.
  • I receive WebSocket notifications whenever some state changes, but while my extension wasn’t running, that state may have changed. I need to do one manual fetch from a server to get up to date.
  • My extension tracks something about the browser state (e.g open tabs) and I need to look over everything to see what happened before this extension started running.
  • My extension has a login system using chrome.identity / something custom and I want to show the user a login prompt when the browser starts/my extension is installed/the user enables my extension if they are not signed in.

I feel like this shows the wide range of use cases where registering for runtime.onInstalled, runtime.onStartup and runtime.onEnabled would be useful and why having a single event could be beneficial.

If anyone can think of anything I've missed, I'd be curious to know.

@hanguokai
Copy link
Member Author

[Oliver] Would personally be interested in an event to transition from non-running to running, but there were reservations about it due to the questions about the usefulness of this.

Doing some one-time work at application startup (rather than checking the status every time later) is a very common programming pattern, regardless of programming language or programming environment. So, I think that's the obvious thing, and there's no need to add more use cases to prove it's useful.

@oliverdunk
Copy link
Member

Doing some one-time work at application startup (rather than checking the status every time later) is a very common programming pattern, regardless of programming language or programming environment. So, I think that's the obvious thing, and there's no need to add more use cases to prove it's useful.

While I agree it would be useful, I think there's still general uncertainty about if we add a generic event or just runtime.onEnabled, so I think we still have more people to convince :)

@RealAlphabet
Copy link

RealAlphabet commented Jun 9, 2023

I've given a lot of thought to all the proposals here. To recap, two things have been proposed.

  1. An additional event such as onEnabled to determine that the extension has been reactivated by the user.

  2. A generic event that is triggered for the following three situations: onInstalled, onEnabled, onStartup.

What I like about the first proposal is that it's a natural pattern for developers who know about onInstalled, onStartup. I was a little confused to see that onEnabled didn't exist, and I think I'm not the only one.

The second case is very interesting because, as Oliver pointed out, the vast majority of chrome extensions that want to subscribe to onEnabled will also subscribe to onInstalled and onStartup to perform (generally) the same code logic.

Another issue raised here that seems important to take into account in the final implementation decision is that an extension won't receive the onInstalled update event if it was deactivated during the update.

I'm in favor of the generic event, as it can be error-prone to have to subscribe to the three events mentioned above. This will prevent the developers from forgetting one of the extension's states by proposing a higher-level event. We can incorporate an object containing the reason parameter with the values "installed", "enabled" and "startup". If reason is "install" then we can add a second parameter updated_from with the version number of the previous version of the extension.

PS: By the way, I must have missed it in the Chrome documentation, but is onStartup triggered when the extension is installed?

@hanguokai
Copy link
Member Author

I think there's still general uncertainty about if we add a generic event or just runtime.onEnabled

ExtensionStartup(D) = onInstalled(A) + onStartup(B) + onEnabled(C)

At present, A and B already exist, while C and D do not exist yet.

If we're not sure whether we should implement C or D , can we implement both? Is there any reason to implement only one of them?

@RealAlphabet
Copy link

I think there's still general uncertainty about if we add a generic event or just runtime.onEnabled

ExtensionStartup(D) = onInstalled(A) + onStartup(B) + onEnabled(C)

At present, A and B already exist, while C and D do not exist yet.

If we're not sure whether we should implement C or D , can we implement both? Is there any reason to implement only one of them?

Yes indeed, I forgot to mention it in my previous comment but I also wondered about it.

@oliverdunk
Copy link
Member

If we're not sure whether we should implement C or D , can we implement both? Is there any reason to implement only one of them?

On the Chrome side, we've had some more discussions and are supportive of implementing both events.

We're tentatively thinking that runtime.onExtensionLoaded would be a better name for runtime.onExtensionStartup. That's likely a good discussion point for the next meeting.

@oliverdunk oliverdunk added the supportive: chrome Supportive from Chrome label Jun 15, 2023
@hanguokai
Copy link
Member Author

On the Chrome side, we've had some more discussions and are supportive of implementing both events.

Good news, thanks for pushing this forward.

@mnoorenberghe
Copy link
Member

While implementing C and D, can we please make guarantees about their listeners being called before any other listeners run? People seem to incorrectly assume that's the case. See #464

@oliverdunk
Copy link
Member

oliverdunk commented Oct 6, 2023

While implementing C and D, can we please make guarantees about their listeners being called before any other listeners run? People seem to incorrectly assume that's the case. See #464

Could you share a more specific behaviour that you're looking for? I would agree with the thoughts in this comment that it seems like things are mostly working as intended.

Edit: Thinking about the other issue some more, making sure C and D always fire after onInstalled is an interesting idea. I'd need to check with the Chrome team on that, in particular to see if there are other ways to handle this, but it sounds sensible at first glance.

@tophf
Copy link

tophf commented Dec 17, 2023

A new name onSessionStart was suggested in #509 to reuse the concept of session used by other parts of the API, listed as "A" in persistence-of-states.md.

Regardless of the name, it might be worth reusing onInstalled's parameter so that the event supersedes the old onStartup/onInstalled with reason being "enabled" | "installed" | "updated" | "profileStart", etc.

@bershanskiy
Copy link
Member

Chromium issue 1513560 Extension API Modification: chrome.runtime.{onEnabled,onExtensionLoaded} might be related.

@oliverdunk
Copy link
Member

Chromium issue 1513560 Extension API Modification: chrome.runtime.{onEnabled,onExtensionLoaded} might be related.

Yes, we have started looking at what it would take to implement this :)

Regardless of the name, it might be worth reusing onInstalled's parameter so that the event supersedes the old onStartup/onInstalled with reason being "enabled" | "installed" | "updated" | "profileStart", etc.

Definitely supportive of having some reasons, we have something similar to this in our Chrome API proposal (which we do for our own processes but doesn't replace conversation in the WECG). Hopefully we can make that public soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement or change to an existing feature supportive: chrome Supportive from Chrome supportive: firefox Supportive from Firefox supportive: safari Supportive from Safari
Projects
None yet
Development

No branches or pull requests

9 participants