Closed Bug 1553454 Opened 1 year ago Closed 1 year ago

Deleting history data fails on GeckoView

Categories

(Toolkit :: Data Sanitization, defect, P3)

All
Android
defect

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox67 --- wontfix
firefox67.0.1 --- wontfix
firefox68 --- wontfix
firefox69 --- fixed

People

(Reporter: esawin, Assigned: esawin)

References

Details

Attachments

(1 file)

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.

Blocks: 1501108
Blocks: 1489669
No longer blocks: 1501108

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

Flags: needinfo?(amarchesini)

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.

Whiteboard: [geckoview:fenix]?

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

Flags: needinfo?(amarchesini)

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.

Priority: -- → P3
Whiteboard: [geckoview:fenix]?

(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!

Flags: needinfo?(mak77)
Flags: needinfo?(esawin)

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.

Flags: needinfo?(mak77)

(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.

Flags: needinfo?(esawin)

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.

Attachment #9067336 - Attachment description: Bug 1553454 - [1.0] Only enable handling of CLEAR_HISTORY when Places is supported. → Bug 1553454 - [1.1] Only enable handling of CLEAR_HISTORY when Places is supported.

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

(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.

Pushed by esawin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/850a0f2e94b2
[1.1] Only enable handling of CLEAR_HISTORY when Places is supported. r=baku
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
You need to log in before you can comment on or make changes to this bug.