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

Proposal: Event priority in Background Service Worker or Event page #464

Open
erosman opened this issue Oct 5, 2023 · 9 comments
Open
Labels
enhancement Enhancement or change to an existing feature proposal Proposal for a change or new feature

Comments

@erosman
Copy link

erosman commented Oct 5, 2023

It appears that currently, there is no specific order that can be relied on. For example:

import {App} from './app.js';

class Process {

  static {
    this.showHelp = false;
    this.init();
  }

  static async init() {
    // preparations
    // can be synchronous or asynchronous
    
    // show help with additional info (after migrate & sync)
    if (this.showHelp) {
      browser.tabs.create({url: '/content/options.html?help'});
      this.showHelp = false;
    }
  }
}

browser.runtime.onInstalled.addListener(details => {
  ['install', 'update'].includes(details.reason) && (Process.showHelp = true);
});

There is no guarantee that by the time if (this.showHelp) is reached, onInstalled event has fired.

It might be worthwhile to consider an event priority in the background script, especially for runtime.onInstalled. For example:

  1. Process import
  2. Fire runtime.onInstalled
  3. Process the rest of the code

Update

Please refer to #464 (comment) for the updated proposal.

@mnoorenberghe
Copy link
Member

I was just fixing a race condition caused by this in Chrome yesterday. Our extension clears some data on extension update since it's version-specific but new data was already written before the runtime.onInstalled listener ran. It's hard to guarantee this function runs first since there isn't always a listener run on every startup due to #353 not being fixed. The solution to that issue should also have a guaranteed order before other code.

@tophf
Copy link

tophf commented Oct 5, 2023

Event listeners are triggered after the background script's first event loop task fully completes. This is consistent with the behavior of service workers per the specification (which was derived from extensions background scripts). AFAIK, extensions always behave like that. In your example your static code is a part of the first event loop task, so it always runs first. The event listeners run either at the beginning of the next event loop task or at the very end of the first one, after the microtask queue fully depletes.

Changing this behavior would arguably break a lot of extensions, so this should be an opt-in. Modern JS frameworks have something similar: when watching an observable you can specify that your callback will run right now, immediately.

@oliverdunk
Copy link
Member

I would agree with @tophf's comment that this seems to be working as intended. @mnoorenberghe, that definitely seems like an interesting use case, happy to keep discussing in the other issue.

@hanguokai
Copy link
Member

It might be worthwhile to consider an event priority in the background script, especially for runtime.onInstalled. For example:

  1. Process import
  2. Fire runtime.onInstalled
  3. Process the rest of the code

The execution order is determined by the JavaScript language itself (the language specification and implementation), there is no magic. So if you place runtime.onInstalled at the end of the code, the order above (2 and 3) is impossible.

Web service worker has two lifecycle (installation or upgrade) related events, self.addEventListener('install', listener) and self.addEventListener('activate', listener). This process is carefully designed. They are very suitable for performing data update logic during upgrades. But they may be different and redundant in the lifecycle of Extension service worker.

If you really want to ensure the priority mentioned above, I think of two proposals.

  1. add an option for running the listener immediately
// some code run before

// run the listener here immediately in the 1st event loop
browser.runtime.onInstalled.addListener(listener, { runImmediately: true});

// some code run after
  1. The extension platform provides a runtime status for extension service worker
// some code run before

/* 
 This state is immutable during one extension SW lifecycle. The values may be
 - 'install': after installation, SW starts for the first time
 - 'update': after upgrade, SW starts for the first time
 - 'enable': after enabling, SW starts for the first time
 - 'update&enable': the extension was disabled and updated, now it is enabled and starts for the first time.
 - 'normal': SW starts except for the above situations
*/
const state  = browser.runtime.SW.state; 

if (state == 'update' || state == 'update&enable') {
    // do something
}

// some code run after

@erosman
Copy link
Author

erosman commented Oct 6, 2023

Preamble

Logically, addListener is meant for events that are going to occur in future. By the time a background JavaScript is executed, events such as 'install', 'update', & 'enable' have already occurred.

Proposal

Since some of the values for onInstalled.addListener are determined prior to loading (or reloading) the extension by the browser, it might be reasonable to supply them synchronously.
It might also eliminate some of the issues currently experienced with runtime.onInstalled.addListener.

  • Values established before loading an extension:

    • install
    • update
    • enable
    • normal
  • Values established after loading an extension (require an event listener)

    • uninstall
    • disable

The same API can be used e.g.

const state = browser.runtime.onInstalled.state; 

if (state === 'instal' || state === 'update') {
    // do something
}

@dotproto
Copy link
Member

I’d be curious whether or not the current asynchronous approach is required for implementation specific reasons, but I’ll have to deferred to browser engineers to answer that question.

Given the current platform capabilities. The following should address the basic pattern described in @erosman’s last comment. I should note that I’m currently on my phone on an airplane, so I have not tested this logic.

const statePromise = new Promise((resolve, reject) => {
  const installHandler = (reason) => {
    if (browser.runtime.lastError) {
      return reject(browser.runtime.lastError);
    }

    resolve(reason);
    browser.runtime.onInstalled.removeListener(installHandler);
  }
});

async main() {
  let state = await statePromise;

  if (state === 'install' || state === 'update') {
    // do something
  }
}
main();

@dotproto dotproto added enhancement Enhancement or change to an existing feature proposal Proposal for a change or new feature and removed needs-triage labels Oct 21, 2023
@oliverdunk
Copy link
Member

I think @hanguokai's comment is a pretty good summary of where we're at today. @dotproto's snippet is a good workaround that should hopefully provide a solution in the short term.

I think the ideas around a runImmediately flag or some state that can be read from the chrome global are interesting, but I haven't given those much thought yet. I'm not aware of a reason why they couldn't be implemented but would definitely still want to check with the engineering team (and we'd need to chat more in the group about what we actually want).

@erosman
Copy link
Author

erosman commented Oct 26, 2023

@dotproto's solution ....

runtime.onInstalled.addListener does not fire on every extension start, enable, and service worker restart.
Therefore, running the main function subsequent to the firing of the aforementioned event is not reliable.
Furthermore, there is a specific sequence which can not be guaranteed under the current system.

Here is a simple example ...

let state;

// Note: runtime.onInstalled.addListener does not fire every time a service-worker/Event-page is executed
browser.runtime.onInstalled.addListener(details => {
  // set state
  state = details.reason; 

  if (details.reason === 'instal' || details.reason === 'update') {
    // do something
  }
});

async function main() {
  // this MUST run BEFORE checking for onInstalled.addListener result
  let pref = await browser.storage.local.get();
  // do something else

  // this MUST run AFTER checking for onInstalled.addListener result
  // do something to the pref only if onInstalled.addListener  has fired and state === 'update'
  if (state === 'update') {
    // do something to pref
    pref = ImportedMigrate(pref);
  }

  // this MUST run AFTER all above
  // send pref to an imported function
  ImportedFunction(pref);
}

main();

@hanguokai's solution ....

Solution 1

browser.runtime.onInstalled.addListener(listener, {runImmediately: true});

Besides the conceptual ambiguity of a listener that behaves as a synchronous function, it would necessitate a 2nd listener.

// check if the reason was install, update, or enable
browser.runtime.onInstalled.addListener(listener, {runImmediately: true});

// need another listener for uninstall, disable
browser.runtime.onInstalled.addListener(listener2);

Solution 2

Providing synchronous data, is similar to the amended proposal in #464 (comment).
Providing it under the same onInstalled namespace should make it simpler and applicable to both service-workers & event-pages.

@hanguokai
Copy link
Member

// need another listener for uninstall, disable

I guess that browsers would not allow the uninstall or disable event for an extension itself.

Solution 2

Yes, your amended proposal is similar to my proposal that I submitted 30 minutes before you. This is a feasible solution, regardless of what name used. Note: there is a "update&enable" state. In that case, the two states overlap.

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 proposal Proposal for a change or new feature
Projects
None yet
Development

No branches or pull requests

6 participants