Skip to content

Fix App Check state setting and promise clearing #6740

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

Merged
merged 6 commits into from
Nov 3, 2022
Merged

Conversation

hsubox76
Copy link
Contributor

@hsubox76 hsubox76 commented Oct 31, 2022

Fixes #6734 (probably)?

Based on solution suggested in #6735

setState() and state usage is already pretty inconsistent throughout the App Check codebase (sometimes immutable, sometimes mutated) and I don't think the best answer is to continue mixing. Additionally, the way setState() is used depends on extending a temporary version of state that, due to a lot of async code, may be stale by the time it setState() is called.

set() is now only done once per key (a FirebaseApp) on the APP_CHECK_STATES, on initialization. Changed setState() to setIInitialState() to make this clear. The Map will now only be used to store a reference to this object, which should never be overwritten. Any further changes can be made by mutating this object's properties. getState() is changed to getStateReference() to make it clear it will just get you a reference to this object.

This seems like the best way to deal with instantly setting state on an object when it can be done through any number of dangling promises.

@changeset-bot
Copy link

changeset-bot bot commented Oct 31, 2022

🦋 Changeset detected

Latest commit: 94f9785

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@firebase/app-check Patch
@firebase/app-check-compat Patch
firebase Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@hsubox76 hsubox76 marked this pull request as ready for review October 31, 2022 18:04
@hsubox76 hsubox76 requested a review from egilmorez as a code owner October 31, 2022 18:04
@google-oss-bot
Copy link
Contributor

google-oss-bot commented Oct 31, 2022

Size Report 1

Affected Products

  • @firebase/app-check

    TypeBase (e2a90bf)Merge (778a3b0)Diff
    browser25.6 kB25.2 kB-365 B (-1.4%)
    esm530.3 kB30.0 kB-253 B (-0.8%)
    main31.5 kB31.1 kB-385 B (-1.2%)
    module25.6 kB25.2 kB-365 B (-1.4%)
  • bundle

    TypeBase (e2a90bf)Merge (778a3b0)Diff
    app-check (CustomProvider)36.5 kB36.2 kB-282 B (-0.8%)
    app-check (ReCaptchaEnterpriseProvider)38.8 kB38.5 kB-364 B (-0.9%)
    app-check (ReCaptchaV3Provider)38.8 kB38.4 kB-364 B (-0.9%)
  • firebase

    TypeBase (e2a90bf)Merge (778a3b0)Diff
    firebase-app-check-compat.js23.1 kB22.7 kB-422 B (-1.8%)
    firebase-app-check.js21.8 kB21.4 kB-452 B (-2.1%)
    firebase-compat.js740 kB740 kB-430 B (-0.1%)

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/XILDHHD9kb.html

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Oct 31, 2022

Size Analysis Report 1

Affected Products

  • @firebase/app-check

    • CustomProvider

      Size

      TypeBase (e2a90bf)Merge (778a3b0)Diff
      size7.72 kB7.52 kB-206 B (-2.7%)
      size-with-ext-deps25.3 kB25.1 kB-212 B (-0.8%)

      Dependency

      TypeBase (e2a90bf)Merge (778a3b0)Diff
      functions

      21 dependencies

      addTokenListener
      computeKey
      createTokenRefresher
      ensureActivated
      factory
      formatDummyToken
      getDBPromise
      getState
      getToken$2
      initTokenRefresher
      internalFactory
      isValid
      makeDummyTokenResult
      notifyTokenListeners
      registerAppCheck
      removeTokenListener
      setState
      sleep
      write
      writeTokenToIndexedDB
      writeTokenToStorage

      20 dependencies

      addTokenListener
      computeKey
      createTokenRefresher
      ensureActivated
      factory
      formatDummyToken
      getDBPromise
      getStateReference
      getToken$2
      initTokenRefresher
      internalFactory
      isValid
      makeDummyTokenResult
      notifyTokenListeners
      registerAppCheck
      removeTokenListener
      sleep
      write
      writeTokenToIndexedDB
      writeTokenToStorage

      + getStateReference
      - getState
      - setState

    • ReCaptchaEnterpriseProvider

      Size

      TypeBase (e2a90bf)Merge (778a3b0)Diff
      size11.3 kB11.0 kB-288 B (-2.6%)
      size-with-ext-deps28.7 kB28.4 kB-294 B (-1.0%)

      Dependency

      TypeBase (e2a90bf)Merge (778a3b0)Diff
      functions

      34 dependencies

      addTokenListener
      computeKey
      createTokenRefresher
      ensureActivated
      exchangeToken
      factory
      formatDummyToken
      getDBPromise
      getDurationString
      getExchangeRecaptchaEnterpriseTokenRequest
      getRecaptcha
      getState
      getToken$1
      getToken$2
      initTokenRefresher
      initializeEnterprise
      internalFactory
      isValid
      loadReCAPTCHAEnterpriseScript
      makeDiv
      makeDummyTokenResult
      notifyTokenListeners
      pad
      queueWidgetRender
      registerAppCheck
      removeTokenListener
      renderInvisibleWidget
      setBackoff
      setState
      sleep
      throwIfThrottled
      write
      writeTokenToIndexedDB
      writeTokenToStorage

      33 dependencies

      addTokenListener
      computeKey
      createTokenRefresher
      ensureActivated
      exchangeToken
      factory
      formatDummyToken
      getDBPromise
      getDurationString
      getExchangeRecaptchaEnterpriseTokenRequest
      getRecaptcha
      getStateReference
      getToken$1
      getToken$2
      initTokenRefresher
      initializeEnterprise
      internalFactory
      isValid
      loadReCAPTCHAEnterpriseScript
      makeDiv
      makeDummyTokenResult
      notifyTokenListeners
      pad
      queueWidgetRender
      registerAppCheck
      removeTokenListener
      renderInvisibleWidget
      setBackoff
      sleep
      throwIfThrottled
      write
      writeTokenToIndexedDB
      writeTokenToStorage

      + getStateReference
      - getState
      - setState

    • ReCaptchaV3Provider

      Size

      TypeBase (e2a90bf)Merge (778a3b0)Diff
      size11.2 kB11.0 kB-288 B (-2.6%)
      size-with-ext-deps28.7 kB28.4 kB-294 B (-1.0%)

      Dependency

      TypeBase (e2a90bf)Merge (778a3b0)Diff
      functions

      34 dependencies

      addTokenListener
      computeKey
      createTokenRefresher
      ensureActivated
      exchangeToken
      factory
      formatDummyToken
      getDBPromise
      getDurationString
      getExchangeRecaptchaV3TokenRequest
      getRecaptcha
      getState
      getToken$1
      getToken$2
      initTokenRefresher
      initializeV3
      internalFactory
      isValid
      loadReCAPTCHAV3Script
      makeDiv
      makeDummyTokenResult
      notifyTokenListeners
      pad
      queueWidgetRender
      registerAppCheck
      removeTokenListener
      renderInvisibleWidget
      setBackoff
      setState
      sleep
      throwIfThrottled
      write
      writeTokenToIndexedDB
      writeTokenToStorage

      33 dependencies

      addTokenListener
      computeKey
      createTokenRefresher
      ensureActivated
      exchangeToken
      factory
      formatDummyToken
      getDBPromise
      getDurationString
      getExchangeRecaptchaV3TokenRequest
      getRecaptcha
      getStateReference
      getToken$1
      getToken$2
      initTokenRefresher
      initializeV3
      internalFactory
      isValid
      loadReCAPTCHAV3Script
      makeDiv
      makeDummyTokenResult
      notifyTokenListeners
      pad
      queueWidgetRender
      registerAppCheck
      removeTokenListener
      renderInvisibleWidget
      setBackoff
      sleep
      throwIfThrottled
      write
      writeTokenToIndexedDB
      writeTokenToStorage

      + getStateReference
      - getState
      - setState

    • getToken

      Size

      TypeBase (e2a90bf)Merge (778a3b0)Diff
      size7.35 kB7.15 kB-206 B (-2.8%)
      size-with-ext-deps24.5 kB24.2 kB-212 B (-0.9%)

      Dependency

      TypeBase (e2a90bf)Merge (778a3b0)Diff
      functions

      22 dependencies

      addTokenListener
      computeKey
      createTokenRefresher
      ensureActivated
      factory
      formatDummyToken
      getDBPromise
      getState
      getToken
      getToken$2
      initTokenRefresher
      internalFactory
      isValid
      makeDummyTokenResult
      notifyTokenListeners
      registerAppCheck
      removeTokenListener
      setState
      sleep
      write
      writeTokenToIndexedDB
      writeTokenToStorage

      21 dependencies

      addTokenListener
      computeKey
      createTokenRefresher
      ensureActivated
      factory
      formatDummyToken
      getDBPromise
      getStateReference
      getToken
      getToken$2
      initTokenRefresher
      internalFactory
      isValid
      makeDummyTokenResult
      notifyTokenListeners
      registerAppCheck
      removeTokenListener
      sleep
      write
      writeTokenToIndexedDB
      writeTokenToStorage

      + getStateReference
      - getState
      - setState

    • initializeAppCheck

      Size

      TypeBase (e2a90bf)Merge (778a3b0)Diff
      size11.2 kB10.9 kB-276 B (-2.5%)
      size-with-ext-deps35.5 kB35.2 kB-282 B (-0.8%)

      Dependency

      TypeBase (e2a90bf)Merge (778a3b0)Diff
      functions

      35 dependencies

      _activate
      addTokenListener
      computeKey
      createTokenRefresher
      ensureActivated
      exchangeToken
      factory
      formatDummyToken
      getDBPromise
      getDebugState
      getDebugToken
      getExchangeDebugTokenRequest
      getState
      getToken$2
      initTokenRefresher
      initializeAppCheck
      initializeDebugMode
      internalFactory
      isDebugMode
      isValid
      makeDummyTokenResult
      notifyTokenListeners
      read
      readDebugTokenFromIndexedDB
      readOrCreateDebugTokenFromStorage
      readTokenFromIndexedDB
      readTokenFromStorage
      registerAppCheck
      removeTokenListener
      setState
      sleep
      write
      writeDebugTokenToIndexedDB
      writeTokenToIndexedDB
      writeTokenToStorage

      35 dependencies

      _activate
      addTokenListener
      computeKey
      createTokenRefresher
      ensureActivated
      exchangeToken
      factory
      formatDummyToken
      getDBPromise
      getDebugState
      getDebugToken
      getExchangeDebugTokenRequest
      getStateReference
      getToken$2
      initTokenRefresher
      initializeAppCheck
      initializeDebugMode
      internalFactory
      isDebugMode
      isValid
      makeDummyTokenResult
      notifyTokenListeners
      read
      readDebugTokenFromIndexedDB
      readOrCreateDebugTokenFromStorage
      readTokenFromIndexedDB
      readTokenFromStorage
      registerAppCheck
      removeTokenListener
      setInitialState
      sleep
      write
      writeDebugTokenToIndexedDB
      writeTokenToIndexedDB
      writeTokenToStorage

      + getStateReference
      + setInitialState
      - getState
      - setState

    • onTokenChanged

      Size

      TypeBase (e2a90bf)Merge (778a3b0)Diff
      size7.45 kB7.24 kB-206 B (-2.8%)
      size-with-ext-deps24.5 kB24.3 kB-212 B (-0.9%)

      Dependency

      TypeBase (e2a90bf)Merge (778a3b0)Diff
      functions

      22 dependencies

      addTokenListener
      computeKey
      createTokenRefresher
      ensureActivated
      factory
      formatDummyToken
      getDBPromise
      getState
      getToken$2
      initTokenRefresher
      internalFactory
      isValid
      makeDummyTokenResult
      notifyTokenListeners
      onTokenChanged
      registerAppCheck
      removeTokenListener
      setState
      sleep
      write
      writeTokenToIndexedDB
      writeTokenToStorage

      21 dependencies

      addTokenListener
      computeKey
      createTokenRefresher
      ensureActivated
      factory
      formatDummyToken
      getDBPromise
      getStateReference
      getToken$2
      initTokenRefresher
      internalFactory
      isValid
      makeDummyTokenResult
      notifyTokenListeners
      onTokenChanged
      registerAppCheck
      removeTokenListener
      sleep
      write
      writeTokenToIndexedDB
      writeTokenToStorage

      + getStateReference
      - getState
      - setState

    • setTokenAutoRefreshEnabled

      Size

      TypeBase (e2a90bf)Merge (778a3b0)Diff
      size7.48 kB7.23 kB-246 B (-3.3%)
      size-with-ext-deps24.6 kB24.3 kB-253 B (-1.0%)

      Dependency

      TypeBase (e2a90bf)Merge (778a3b0)Diff
      functions

      22 dependencies

      addTokenListener
      computeKey
      createTokenRefresher
      ensureActivated
      factory
      formatDummyToken
      getDBPromise
      getState
      getToken$2
      initTokenRefresher
      internalFactory
      isValid
      makeDummyTokenResult
      notifyTokenListeners
      registerAppCheck
      removeTokenListener
      setState
      setTokenAutoRefreshEnabled
      sleep
      write
      writeTokenToIndexedDB
      writeTokenToStorage

      21 dependencies

      addTokenListener
      computeKey
      createTokenRefresher
      ensureActivated
      factory
      formatDummyToken
      getDBPromise
      getStateReference
      getToken$2
      initTokenRefresher
      internalFactory
      isValid
      makeDummyTokenResult
      notifyTokenListeners
      registerAppCheck
      removeTokenListener
      setTokenAutoRefreshEnabled
      sleep
      write
      writeTokenToIndexedDB
      writeTokenToStorage

      + getStateReference
      - getState
      - setState

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/VEnAaWpuQ7.html

@ishowta
Copy link

ishowta commented Nov 1, 2022

I think it is better than my code, but it seems to be prone to bugs because the information rewritten in the setter is not immediately reflected in the local state.

Performance may be somewhat degraded, but I thought it would be safer and more readable to stop using setter and getter and use mutex to execute the entire function exclusively.

@hsubox76
Copy link
Contributor Author

hsubox76 commented Nov 1, 2022

That's true, we would have to use getState() before every reference to state to be sure. I still think whichever solution we go with should be global and not a mix of mutable and immutable state so perhaps state should just be rewritten to be directly mutable across the board, I'll take a look at doing that.

Copy link
Contributor

@dwyfrequency dwyfrequency left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@hsubox76 hsubox76 merged commit 457fc2e into master Nov 3, 2022
@hsubox76 hsubox76 deleted the ch-appcheck-fix-2 branch November 3, 2022 17:05
@google-oss-bot google-oss-bot mentioned this pull request Nov 8, 2022
@firebase firebase locked and limited conversation to collaborators Dec 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sometimes old AppCheck tokens continue to be returned.
4 participants