diff --git a/apps/weblibre/lib/features/geckoview/features/browser/domain/services/browser_data.dart b/apps/weblibre/lib/features/geckoview/features/browser/domain/services/browser_data.dart index 8c376fc6..2ba11202 100644 --- a/apps/weblibre/lib/features/geckoview/features/browser/domain/services/browser_data.dart +++ b/apps/weblibre/lib/features/geckoview/features/browser/domain/services/browser_data.dart @@ -17,8 +17,10 @@ * You should have received a copy of the GNU Affero General Public License * along with this program. If not, see . */ +import 'package:flutter/foundation.dart'; import 'package:flutter_mozilla_components/flutter_mozilla_components.dart'; import 'package:riverpod_annotation/riverpod_annotation.dart'; +import 'package:weblibre/core/logger.dart'; import 'package:weblibre/features/bangs/domain/repositories/data.dart'; import 'package:weblibre/features/geckoview/features/tabs/data/providers.dart'; import 'package:weblibre/features/user/data/models/general_settings.dart'; @@ -27,9 +29,17 @@ part 'browser_data.g.dart'; @Riverpod(keepAlive: true) class BrowserDataService extends _$BrowserDataService { - final _service = GeckoDeleteBrowserDataService(); + /// The [service] seam is test-only injection; production always constructs + /// the default. Kept `final` so the live singleton's backing service can't be + /// swapped at runtime. + @visibleForTesting + BrowserDataService({GeckoDeleteBrowserDataService? service}) + : _service = service ?? GeckoDeleteBrowserDataService(); + + final GeckoDeleteBrowserDataService _service; var _onStartDeleted = false; + var _onStartContainerDataCleared = false; Future deleteDataOnEngineStart( Set? types, @@ -73,14 +83,32 @@ class BrowserDataService extends _$BrowserDataService { return _service.clearDataForContext(contextId); } - Future clearContainerDataOnEngineStart(List contextIds) async { - if (!_onStartDeleted && contextIds.isNotEmpty) { - for (final contextId in contextIds) { + /// Clears Gecko session-context data for every [contextId], best-effort: a + /// single failing context is logged and skipped so it never blocks the rest. + /// + /// Not gated by the one-shot startup guard — used both by the guarded + /// [clearContainerDataOnEngineStart] fallback and directly on explicit Quit. + Future clearContainerData(List contextIds) async { + for (final contextId in contextIds) { + try { await clearDataForContext(contextId); + } catch (e, st) { + logger.e( + 'Failed to clear data for container context $contextId', + error: e, + stackTrace: st, + ); } } } + Future clearContainerDataOnEngineStart(List contextIds) async { + if (!_onStartContainerDataCleared && contextIds.isNotEmpty) { + _onStartContainerDataCleared = true; + await clearContainerData(contextIds); + } + } + @override void build() {} } diff --git a/apps/weblibre/lib/features/geckoview/features/browser/domain/services/browser_data.g.dart b/apps/weblibre/lib/features/geckoview/features/browser/domain/services/browser_data.g.dart index d60f77b8..b9e8a004 100644 --- a/apps/weblibre/lib/features/geckoview/features/browser/domain/services/browser_data.g.dart +++ b/apps/weblibre/lib/features/geckoview/features/browser/domain/services/browser_data.g.dart @@ -42,7 +42,7 @@ final class BrowserDataServiceProvider } String _$browserDataServiceHash() => - r'a317f0d56d1bfc61dc99af1e60080aaa248d6eed'; + r'2df2f652342efc3e16606b92fdef6062b02f72df'; abstract class _$BrowserDataService extends $Notifier { void build(); diff --git a/apps/weblibre/lib/features/geckoview/features/tabs/presentation/screens/container_edit.dart b/apps/weblibre/lib/features/geckoview/features/tabs/presentation/screens/container_edit.dart index 55ebacf5..4e457f09 100644 --- a/apps/weblibre/lib/features/geckoview/features/tabs/presentation/screens/container_edit.dart +++ b/apps/weblibre/lib/features/geckoview/features/tabs/presentation/screens/container_edit.dart @@ -122,31 +122,33 @@ class ContainerEditScreen extends HookConsumerWidget { name: name.isNotEmpty ? name : null, color: selectedColor.value, isPinned: isPinned.value, - metadata: initialContainer.metadata.copyWith( - contextualIdentity: contextualIdentity.value, - iconData: selectedIcon.value, - proxyConnectionId: contextualIdentity.value != null - ? proxyConnectionId.value - : null, - clearDataOnExit: - clearDataOnExit.value && contextualIdentity.value != null, - excludeFromIndex: excludeFromIndex.value, - // Requires a Gecko contextId: the native delegate can't hard-exclude - // a container's visits without one. sanitized() enforces the same - // invariant defensively on write. - excludeFromHistory: - excludeFromHistory.value && contextualIdentity.value != null, - bypassGlobalProxy: - contextualIdentity.value != null && - proxyConnectionId.value == null && - bypassGlobalProxy.value, - useCustomColor: useCustomColor.value, - assignedSites: assignedSites.value, - // Strict mode requires a Gecko contextId (the extension keys - // strictness on the tab's cookieStoreId). sanitized() enforces the - // same invariant defensively on write. - strictMode: strictMode.value && contextualIdentity.value != null, - ).sanitized(), + metadata: initialContainer.metadata + .copyWith( + contextualIdentity: contextualIdentity.value, + iconData: selectedIcon.value, + proxyConnectionId: contextualIdentity.value != null + ? proxyConnectionId.value + : null, + clearDataOnExit: + clearDataOnExit.value && contextualIdentity.value != null, + excludeFromIndex: excludeFromIndex.value, + // Requires a Gecko contextId: the native delegate can't hard-exclude + // a container's visits without one. sanitized() enforces the same + // invariant defensively on write. + excludeFromHistory: + excludeFromHistory.value && contextualIdentity.value != null, + bypassGlobalProxy: + contextualIdentity.value != null && + proxyConnectionId.value == null && + bypassGlobalProxy.value, + useCustomColor: useCustomColor.value, + assignedSites: assignedSites.value, + // Strict mode requires a Gecko contextId (the extension keys + // strictness on the tab's cookieStoreId). sanitized() enforces the + // same invariant defensively on write. + strictMode: strictMode.value && contextualIdentity.value != null, + ) + .sanitized(), ); } @@ -543,7 +545,9 @@ class ContainerEditScreen extends HookConsumerWidget { value: clearDataOnExit.value, title: const Text('Clear Data on Exit'), subtitle: const Text( - 'Clear cookies and site data when app closes', + "Clear cookies and site data for this container's " + 'regular tabs when the app closes. Isolated tabs ' + 'keep separate data.', ), secondary: const Icon(MdiIcons.databaseRemove), onChanged: (contextualIdentity.value != null) @@ -572,7 +576,9 @@ class ContainerEditScreen extends HookConsumerWidget { title: const Text('Exclude from History'), subtitle: Text( contextualIdentity.value != null - ? "Don't record this container's browsing history" + ? "Don't record new visits from this container's " + 'regular tabs. Existing history is kept; ' + 'isolated tabs track separately.' : 'Requires cookie isolation to be enabled', ), secondary: const Icon(MdiIcons.incognito), diff --git a/apps/weblibre/lib/utils/exit_app.dart b/apps/weblibre/lib/utils/exit_app.dart index 3e57804a..e4ebcd2e 100644 --- a/apps/weblibre/lib/utils/exit_app.dart +++ b/apps/weblibre/lib/utils/exit_app.dart @@ -24,6 +24,8 @@ import 'package:flutter_mozilla_components/flutter_mozilla_components.dart'; import 'package:riverpod/riverpod.dart'; import 'package:weblibre/core/database_registry.dart'; import 'package:weblibre/core/logger.dart'; +import 'package:weblibre/features/geckoview/features/browser/domain/services/browser_data.dart'; +import 'package:weblibre/features/geckoview/features/tabs/domain/repositories/container.dart'; import 'package:weblibre/features/geckoview/features/tabs/domain/repositories/tab.dart'; import 'package:weblibre/features/tor/domain/services/tor_proxy.dart'; @@ -41,6 +43,44 @@ Future exitApp(ProviderContainer container) async { logger.e('Failed to close tabs', error: e, stackTrace: st); } + // 1b. Explicit-Quit cleanup for containers with "Clear Data on Exit" enabled. + // Done here — while the databases and Gecko engine are still alive — so the + // clear gets a chance to run on a deliberate Quit rather than only on the + // next launch. + // + // Caveat: GeckoView's session-context clear is fire-and-forget (see + // GeckoDeleteBrowsingDataControllerImpl.clearDataForSessionContext) — it + // has no completion signal, so awaiting it does NOT mean the clear + // finished, only that it was dispatched. Since step 3 tears down the + // GeckoRuntime, we give the dispatched clear a short best-effort window to + // reach Gecko first. The startup fallback in browser_view.dart is retained + // as the actual guarantee — it covers force-stop/process death and this + // window elapsing before the clear lands. + try { + final containersToClear = await container + .read(containerRepositoryProvider.notifier) + .getContainersToClearOnExit(); + + if (containersToClear.isNotEmpty) { + await container + .read(browserDataServiceProvider.notifier) + .clearContainerData(containersToClear); + // Best-effort settle: yield before the engine shutdown in step 3 so the + // fire-and-forget native clear has a chance to be processed by Gecko. + await Future.delayed(const Duration(milliseconds: 500)); + logger.i( + 'Dispatched data clear for ${containersToClear.length} ' + 'on-exit container(s)', + ); + } + } catch (e, st) { + logger.e( + 'Failed to clear container data on exit', + error: e, + stackTrace: st, + ); + } + // 2. Stop Tor proxy (only if it was initialized) if (container.exists(torProxyServiceProvider)) { try { diff --git a/apps/weblibre/test/features/geckoview/features/browser/domain/services/browser_data_test.dart b/apps/weblibre/test/features/geckoview/features/browser/domain/services/browser_data_test.dart new file mode 100644 index 00000000..b3cd7baf --- /dev/null +++ b/apps/weblibre/test/features/geckoview/features/browser/domain/services/browser_data_test.dart @@ -0,0 +1,105 @@ +import 'package:flutter_mozilla_components/flutter_mozilla_components.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:weblibre/features/geckoview/features/browser/domain/services/browser_data.dart'; + +class _FakeGeckoDeleteBrowserDataService extends GeckoDeleteBrowserDataService { + final clearedContexts = []; + + /// Contexts for which [clearDataForContext] should throw (simulating a native + /// failure), so we can assert one failure doesn't block the remaining ones. + final failingContexts = {}; + + @override + Future clearDataForContext(String contextId) async { + clearedContexts.add(contextId); + if (failingContexts.contains(contextId)) { + throw Exception('simulated native failure for $contextId'); + } + } +} + +void main() { + group('BrowserDataService.clearContainerDataOnEngineStart', () { + test( + 'still clears container data after deleteDataOnEngineStart ran ' + '(regression: shared one-shot flag skipped container clearing, #524)', + () async { + final gecko = _FakeGeckoDeleteBrowserDataService(); + final service = BrowserDataService(service: gecko); + + // Startup always runs the global on-start deletion first, even when + // no delete-on-quit types are configured (null). + await service.deleteDataOnEngineStart(null); + await service.clearContainerDataOnEngineStart(['ctx-1', 'ctx-2']); + + expect(gecko.clearedContexts, ['ctx-1', 'ctx-2']); + }, + ); + + test('clears only once per app start', () async { + final gecko = _FakeGeckoDeleteBrowserDataService(); + final service = BrowserDataService(service: gecko); + + await service.clearContainerDataOnEngineStart(['ctx-1']); + await service.clearContainerDataOnEngineStart(['ctx-2']); + + expect(gecko.clearedContexts, ['ctx-1']); + }); + + test( + 'empty context list neither clears nor consumes the one-shot run', + () async { + final gecko = _FakeGeckoDeleteBrowserDataService(); + final service = BrowserDataService(service: gecko); + + await service.clearContainerDataOnEngineStart(const []); + expect(gecko.clearedContexts, isEmpty); + + await service.clearContainerDataOnEngineStart(['ctx-1']); + expect(gecko.clearedContexts, ['ctx-1']); + }, + ); + + test( + 'a single failing context does not block the remaining ones', + () async { + final gecko = _FakeGeckoDeleteBrowserDataService() + ..failingContexts.add('ctx-2'); + final service = BrowserDataService(service: gecko); + + await service.clearContainerDataOnEngineStart([ + 'ctx-1', + 'ctx-2', + 'ctx-3', + ]); + + // ctx-2 threw, but ctx-1 and ctx-3 were still attempted. + expect(gecko.clearedContexts, ['ctx-1', 'ctx-2', 'ctx-3']); + }, + ); + }); + + group('BrowserDataService.clearContainerData (explicit Quit)', () { + test('is not gated by the one-shot startup guard', () async { + final gecko = _FakeGeckoDeleteBrowserDataService(); + final service = BrowserDataService(service: gecko); + + // Consume the startup one-shot guard first. + await service.clearContainerDataOnEngineStart(['startup']); + // Explicit Quit cleanup must still run afterwards. + await service.clearContainerData(['ctx-1', 'ctx-2']); + + expect(gecko.clearedContexts, ['startup', 'ctx-1', 'ctx-2']); + }); + + test('logs and skips a failing context, clearing the rest', () async { + final gecko = _FakeGeckoDeleteBrowserDataService() + ..failingContexts.add('ctx-1'); + final service = BrowserDataService(service: gecko); + + await service.clearContainerData(['ctx-1', 'ctx-2']); + + expect(gecko.clearedContexts, ['ctx-1', 'ctx-2']); + }); + }); +} diff --git a/packages/flutter_mozilla_components/android/src/main/kotlin/eu/weblibre/flutter_mozilla_components/api/GeckoDeleteBrowsingDataControllerImpl.kt b/packages/flutter_mozilla_components/android/src/main/kotlin/eu/weblibre/flutter_mozilla_components/api/GeckoDeleteBrowsingDataControllerImpl.kt index 466c47e3..51f19d0c 100644 --- a/packages/flutter_mozilla_components/android/src/main/kotlin/eu/weblibre/flutter_mozilla_components/api/GeckoDeleteBrowsingDataControllerImpl.kt +++ b/packages/flutter_mozilla_components/android/src/main/kotlin/eu/weblibre/flutter_mozilla_components/api/GeckoDeleteBrowsingDataControllerImpl.kt @@ -188,6 +188,7 @@ class GeckoDeleteBrowsingDataControllerImpl : GeckoDeleteBrowsingDataController ) || dataTypes.contains(ClearDataType.ONLY_CACHES)) ) { callback(Result.failure(Exception("Cookies/Cache must be exclusively!"))) + return@withContext } // Convert ClearDataType to Engine.BrowsingData flags @@ -200,8 +201,10 @@ class GeckoDeleteBrowsingDataControllerImpl : GeckoDeleteBrowsingDataController } }.toIntArray() - // Find tabs on this host (so we can detach their engine sessions and - // also discover any container/contextId partitions that need clearing). + // Find tabs on this host so we can detach their engine sessions + // before clearing (across every container — the base-domain clear + // below covers all partitions, so any open session on this host in + // any container could otherwise re-accumulate cleared data). val matchingTabs = components.core.store.state.allTabs.filter { tab -> val tabHost = runCatching { tab.content.url.toUri().host }.getOrNull() ?: return@filter false @@ -215,22 +218,16 @@ class GeckoDeleteBrowsingDataControllerImpl : GeckoDeleteBrowsingDataController components.core.store.dispatch(EngineAction.UnlinkEngineSessionAction(it.id)) } - // GeckoView's clearDataFromBaseDomain (used by engine.clearData(host=)) - // only targets the default origin attributes partition. Tabs that live - // inside a contextual identity ("container") store their cookies/storage - // under a `geckoViewSessionContextId` origin attribute that the - // base-domain clear does not touch. To make clearing actually effective - // for container tabs, also fire clearDataForSessionContext for any - // contextIds we found. This is broader than ideal (it clears the whole - // container, not just this host) but there is no GeckoView API to - // combine host + context. - val contextIds = matchingTabs.mapNotNull { it.contextId }.toSet() - contextIds.forEach { contextId -> - components.core.runtime.storageController - .clearDataForSessionContext(contextId) - } - - // Clear data for the specific host (default partition). + // Clear data for the specific host only. clearDataFromBaseDomain + // (used by engine.clearData(host=)) deletes the site under an + // empty OriginAttributes pattern, which already matches ALL + // partitions — including every container's + // `geckoViewSessionContextId`. So this clears this host across all + // containers WITHOUT touching other sites in those containers. + // + // Do NOT fall back to clearDataForSessionContext here: that wipes a + // container's entire storage (every unrelated site in it), which is + // the container-wide data-loss reported in #524. components.core.engine.clearData( data = Engine.BrowsingData.select(*browsingDataTypes), host = host, diff --git a/packages/flutter_mozilla_components/android/src/main/kotlin/eu/weblibre/flutter_mozilla_components/middleware/HistoryVisitCorrelationMiddleware.kt b/packages/flutter_mozilla_components/android/src/main/kotlin/eu/weblibre/flutter_mozilla_components/middleware/HistoryVisitCorrelationMiddleware.kt index 3436c77f..af53ff36 100644 --- a/packages/flutter_mozilla_components/android/src/main/kotlin/eu/weblibre/flutter_mozilla_components/middleware/HistoryVisitCorrelationMiddleware.kt +++ b/packages/flutter_mozilla_components/android/src/main/kotlin/eu/weblibre/flutter_mozilla_components/middleware/HistoryVisitCorrelationMiddleware.kt @@ -31,18 +31,22 @@ class HistoryVisitCorrelationMiddleware : next: (BrowserAction) -> Unit, action: BrowserAction, ) { - next(action) - + // Record the correlation BEFORE forwarding the action. A tab's contextId + // is stable across a URL change, so it is already readable here — and Gecko + // can fire the delegate's onVisited for this navigation as soon as the store + // updates. Recording first closes the window where onVisited could resolve + // the visit before its correlation exists (and mis-attribute or leak it). if (action is ContentAction.UpdateUrlAction) { val normalTab = store.state.findNormalTab(action.sessionId) if (normalTab != null) { HistoryVisitCorrelationCache.record(action.url, normalTab.contextId) - return - } - - store.state.findCustomTab(action.sessionId)?.let { customTab -> - HistoryVisitCorrelationCache.record(action.url, customTab.contextId) + } else { + store.state.findCustomTab(action.sessionId)?.let { customTab -> + HistoryVisitCorrelationCache.record(action.url, customTab.contextId) + } } } + + next(action) } }