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

feat: Session#clearData API #40983

Merged
merged 14 commits into from Feb 26, 2024
Merged

feat: Session#clearData API #40983

merged 14 commits into from Feb 26, 2024

Conversation

clavin
Copy link
Member

@clavin clavin commented Jan 12, 2024

This PR adds a new clearData API for clearing out data created by Electron to the Session class.

See the API proposal below for more details, considerations, and future work.

Checklist

Release Notes

Notes: clearData method added to Session.

Proposed API

ses.clearData()

This method corresponds to invoking Chromium's BrowsingDataRemover, which can clear many types of data, inlcuding:

  • Cookies
  • Cache
  • Indexed DB
  • Local storage
  • WebSQL
  • Service workers
  • Background fetch
  • And many others

At present, this method attempt to clear all categories. See the "Future work" section for more information about how this API is designed to be extended.

This method returns a Promise<void> that resolves once the data is cleared successfully, or rejects if any categories of data failed to be cleared with an error.

const win = new BroserWindow(...);
const { session } = win.webContents;

// Attempt to clear all browsing data
try {
  await session.clearData();
} catch (err) {
  console.error("Failed to clear data:", err);
}

Prior Art

While working on this feature, I discovered Chrome exposes this functionality via an extension API: chrome.browsingData extension API. (See alternatives below for more discussion on this existing API.)

Future Work

This API is designed to be extended in the future with an optional options object parameter. The options will allow users to filter what kinds of data to clear, based on the options available from Chromium's BrowsingDataRemover API. This may also allow us to deprecate some other methods whose functionality become completely subsumed by this expanded method behavior, using the right options.

With this richer configuration, we can also return richer information on the error object in the failure case, like which categories of data failed to be removed.

Alternatives

Multiple, more granular methods

Instead of creating one big supermethod with an options object, we could choose to break it up into individual smaller methods (clearCacheData, clearCookiesData, clearIndexedDBData, etc.). We may opt to also keep the general clearData as a shortcut for calling all of these methods together, effectively clearing all data.

The downside is that it's not clear what this granularity would give us. The API surface for Session would become even more noisy.

Different name

The original proposal mirrored the underlying API name: clearBrowsingData. It was decided that it would be more clear to name this method simply clearData. You don't really "browse" with Electron apps, which begs the question of what "browsing data" would be for Electron.

Not rejecting

Instead of rejecting and potentially throwing errors for what might not be a critical issue, we could make the return value of this method a Promise<ClearedDataInfo> that contains information about what categories of data were successfully cleared (not sure that data is useful) and which ones failed.

Reuse the extensions API

We could try to go down the route of exposing the existing chrome.browsingData extension API instead of implementing our own API on top of the same functionality.

However, this approach has some notable disadvantages:

  • Electron's support of Chrome's extensions API seems to be provided as a compatibility layer for reasonable, existing Chrome extensions to work with Electron; whereas, this functionality is useful outside of extension compatibility and it is unlikely that extensions using the Chrome API are built with Electron's use case in mind.
  • The extension API may not be compatible with Electron, or at least parts of it would not translate to Electron properly.
  • We are bound to Chrome's API surface and design, which is built with a different model of trust and a different scope of power from Electron.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jan 12, 2024
@samuelmaddock
Copy link
Member

You don't really "browse" with Electron apps, which begs the question of what "browsing data" would be for Electron. We could change the name of the method to better suit Electron apps. Perhaps just clearData or another name that fits better.

The bit about browsing is a good point and I like the idea of using the name clearData. One thing that helps sway me is the BrowsingDataRemoverImpl::RemoveImpl code allows the embedder application to delete its own data—meaning browsing data plus whatever else we want to cleanup in Electron.

@clavin clavin force-pushed the clavin/session-clear-data-api branch from f796e5a to f78936f Compare January 16, 2024 22:52
@clavin clavin added the semver/minor backwards-compatible functionality label Jan 17, 2024
@clavin clavin changed the title [WIP] feat: Session#clearBrowsingData API feat: Session#clearBrowsingData API Jan 17, 2024
@clavin clavin marked this pull request as ready for review January 17, 2024 22:17
Copy link
Member

@ckerr ckerr left a comment

Choose a reason for hiding this comment

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

This API is designed to be extended in the future with an optional options object parameter. The options will allow users to filter what kinds of data to clear, based on the options available from Chromium's BrowsingDataRemover API. This may also allow us to deprecate some other methods whose functionality become completely subsumed by this expanded method behavior, using the right options.

This is a nice idea @clavin! Thanks for the proposal!

Would it be possible to outline what the full API would look like at endgame? I'm curious about what options would be exposed and what methods would be deprecated.

shell/browser/api/electron_api_session.cc Outdated Show resolved Hide resolved
shell/browser/api/electron_api_session.cc Outdated Show resolved Hide resolved
shell/browser/api/electron_api_session.cc Outdated Show resolved Hide resolved
shell/browser/api/electron_api_session.cc Outdated Show resolved Hide resolved
shell/browser/api/electron_api_session.cc Show resolved Hide resolved
@clavin
Copy link
Member Author

clavin commented Jan 23, 2024

I discovered Chrome exposes an API like the one proposed here via its extensions APIs. I updated the main post with more information and discussion about it.

@clavin
Copy link
Member Author

clavin commented Jan 23, 2024

Would it be possible to outline what the full API would look like at endgame? I'm curious about what options would be exposed and what methods would be deprecated.

@ckerr While answering this question, I discovered the chrome.browsingData extension API, which looks similar to what I had in mind. (See the updated main post for more discussion about that API.)

The future work on this proposed API would look similar to that existing API, but with a change in shape:

The method signature would be simply ses.clearBrowsingData([options]). options is an optional object. When absent, the method should work as it does now in this proposal: clearing everything. The options it would accept would be something like this:

  • dataTypes: The categories of data that should be cleared. This is similar to the dataToRemove parameter and the DataTypeSet in the existing chrome.browsingData extension API. Instead of being an object with all boolean values, this option would accept just an array of strings. E.g. dataTypes: ['cache', 'cookies', 'indexedDB', 'localStorage'].
    • We would have our own set of data type categories that make sense for Electron. For example, the chrome.browsingData category 'passwords' would not be present in Electron as it doesn't store that information.
  • origins and excludeOrigins: only one of these two options is allowed. They indicate whether to clear only specific origins or to clear all except specific origins, and it specifies those origins as well. This design is borrowed from the chrome.browsingData extension API.
  • There should be more options to control the behavior of the options above, like avoidClosingConnections (which is a content::BrowsingDataRemover::DataType, but acts more like a modifier than a category in its own right) and originMatchingMode (corresponding to content::BrowsingDataFilterBuilder::OriginMatchingMode). There may also be others that are discovered while taking on that future work.

(As an aside, the originTypes and since options from the chrome.browsingData API are not expected to show up in near-future work on this proposed API. There's no technical reason they can't be added in the future, as the API surface is capable of supporting more options; however, they either need some work to fit into Electron's API and/or I don't suspect they will be important enough to Electron developers to include in the next iteration.)

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jan 24, 2024
spec/api-session-spec.ts Show resolved Hide resolved
promise_.Resolve();
} else {
promise_.RejectWithErrorMessage(base::StringPrintf(
"Failed to clear browsing data (%" PRIu64 ")", failed_data_types));
Copy link
Member

Choose a reason for hiding this comment

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

It looks like v8::Error can be cast to an object to set additional properties according to the v8 tests.

Maybe the failed data types could be set as a custom property on the error object. It would be a little unusual though as Electron doesn't do this anywhere else.

Copy link
Member Author

Choose a reason for hiding this comment

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

I had that behavior in a previous revision, and then I realized that it would be better if the failed data types was a set or array of data categories as strings. It would be far more useful to JS code in that form. That mapping doesn't really exist right now but might in a future revision.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think there are two approaches to pick from:

  1. Keep this error as-is for now, and extend it in the future when the mapping from data types to category strings exists.
  2. Same as (1) where we add a simple field in the future, but attach an errorCode field now with the raw numerical value of failed_data_types.

There's some rough edges with exposing this raw value to JS though. It's a 64-bit unsigned integer, which could theoretically go beyond the 53-bit limit of normal JS numbers, though there are barely half that number of flags right now. The safe route would be to expose it as a bigint, but that could be confusing.

Copy link
Member

@zcbenz zcbenz left a comment

Choose a reason for hiding this comment

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

What is the difference between this and the clearStorageData API?

@clavin
Copy link
Member Author

clavin commented Feb 12, 2024

@zcbenz The clearStorageData API calls StoragePartition::ClearData directly. The Chromium API backing this proposed clearBrowsingData API (content::BrowsingDataRemover) is more general and includes options for calling StoragePartition::ClearData, among others. For example, a data category not currently covered by existing APIs like clearStorageData is some networking history, which is only called via content::BrowsingDataRemover. I suspect there are others that fall into this category too.

docs/api/session.md Outdated Show resolved Hide resolved
Copy link
Member

@zcbenz zcbenz left a comment

Choose a reason for hiding this comment

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

API LGTM

Copy link
Member

@erickzhao erickzhao left a comment

Choose a reason for hiding this comment

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

API LGTM

@clavin clavin changed the title feat: Session#clearBrowsingData API feat: Session#clearData API Feb 20, 2024
@clavin clavin removed the wip ⚒ label Feb 20, 2024
@clavin
Copy link
Member Author

clavin commented Feb 23, 2024

The API name change came after the formal reviews in this PR; however, the change was supported by API WG members I ran it past (and even in a comment up above), and the ultimate goal is to try deprecating the overlapping clearStorageData in the near future, so I hope that it's okay to keep the existing API reviews for this PR. We have plenty of time to rename in another PR if we ultimately decide to change course.

@ckerr ckerr self-requested a review February 25, 2024 00:17
@zcbenz zcbenz merged commit 12d7a8f into main Feb 26, 2024
15 checks passed
@zcbenz zcbenz deleted the clavin/session-clear-data-api branch February 26, 2024 00:39
Copy link

release-clerk bot commented Feb 26, 2024

Release Notes Persisted

clearData method added to Session.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants