1553454 - Deleting history data fails on GeckoView

Status: RESOLVED FIXED

(Toolkit :: Data Sanitization, defect, P3)

Description

[Eugen Sawin [:esawin]]

With bug 1489669 we're exposing nsIClearDataService through the GeckoView StorageController API.

Testing locally on latest m-c, I've found that Service.clearData.deleteData() fails with [JavaScript Error: "NS_ERROR_FILE_NOT_FOUND: " {file: "resource://gre/modules/ClearDataService.jsm" line: 613}] when the CLEAR_HISTORY flag is set.

Comment 1

[Eugen Sawin [:esawin]]

Baku, is this a GeckoView-specific issue? Is there anything I can do to help diagnose it?

Comment 2

[Chris Peterson [:cpeterson]]

Do we need to fix this bug for GeckoView's Storage Controller API (bug 1489669) to work in Fenix MVP? Adding [geckoview:fenix] whiteboard tag to track this issue until we know.

Comment 3

[Andrea Marchesini [:baku]]

It seems that PlacesUtils.history doesn't work in GeckoView. Would be great if you can debug it.

Comment 4

[Chris Peterson [:cpeterson]]

Removing [geckoview:fenix] whiteboard tag. We don't need to track this bug for Fenix because Eugen has a workaround in the GV API exposed to Fenix.

Comment 5

[Johann Hofmann [:johannh]]

(In reply to Andrea Marchesini [:baku] from comment #3)

It seems that PlacesUtils.history doesn't work in GeckoView. Would be great if you can debug it.

:mak, can you help with this?

(In reply to Chris Peterson [:cpeterson] from comment #4)

Removing [geckoview:fenix] whiteboard tag. We don't need to track this bug for Fenix because Eugen has a workaround in the GV API exposed to Fenix.

What's that workaround? :)

Thanks!

Comment 6

[Marco Bonardo [:mak]]

Isn't GeckoView Android only? Places is a desktop only history implementation, mobile has different implementations (Fenix has a new Rust based one, the Firefox for Android has its own history impl, as well as Firefox for IOS). If you're going to expose nsIClearDataService completely, you must ensure the proper services are used on different devices, PlacesUtils surely doesn't work out of desktop.
Basically, HistoryCleaner in ClearDataService.jsm is currently not cross-platform, someone should add support for the other systems to it.

Comment 7

[Eugen Sawin [:esawin]]

Attachment: Bug 1553454 - [1.1] Only enable handling of CLEAR_HISTORY when Places is supported.

Comment 8

[Eugen Sawin [:esawin]]

(In reply to Johann Hofmann [:johannh] from comment #5)

What's that workaround? :)

I've removed CLEAR_HISTORY from the flags passed by default, see GeckoViewStorageController.jsm changes in the patch here.

(In reply to Marco Bonardo [::mak] from comment #6)

Isn't GeckoView Android only? Places is a desktop only history implementation, mobile has different implementations (Fenix has a new Rust based one, the Firefox for Android has its own history impl, as well as Firefox for IOS). If you're going to expose nsIClearDataService completely, you must ensure the proper services are used on different devices, PlacesUtils surely doesn't work out of desktop.
Basically, HistoryCleaner in ClearDataService.jsm is currently not cross-platform, someone should add support for the other systems to it.

We're not exposing nsIClearDataService completely, the issue was that the catch-all flag CLEAR_ALL included CLEAR_HISTORY so we hit the path with the missing PlacesUtils file.

Comment 9

[Marco Bonardo [:mak]]

What I meant is that, if you plan to use nsIClearDataService across all the platforms, you should check that all of its parts are ready and cross platform. It should not be too hard to add the missing pieces, but considered this bug I think this kind of check wasn't done yet.

Comment 10

[Johann Hofmann [:johannh]]

But, you're still clearing history in some other way then?

Comment 11

[Eugen Sawin [:esawin]]

(In reply to Johann Hofmann [:johannh] from comment #10)

But, you're still clearing history in some other way then?

GeckoView doesn't collect history persistently, we expose an API (HistoryDelegate) for the app to do so, if required.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/850a0f2e94b2
[1.1] Only enable handling of CLEAR_HISTORY when Places is supported. r=baku

Comment 13

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/850a0f2e94b2

Comment 14

[Julien Cristau [:jcristau]]

fix-optional for 68 per comment 4.