1197869 - Clear/update GlobalHistory when clearing history from Clear Private Data or after a sync

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect)

Description

[Richard Newman [:rnewman]]

As far as I can tell, we never blank GlobalHistory's visited link set. That means link highlighting will be stale until the browser is restarted.

We should attempt to throw away this cache whenever appropriate, probably via a LocalBroadcast.

https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/GlobalHistory.java

Comment 1

[Anita]

Hi, I would like to resolve this issue as my first bug.

Comment 2

[Richard Newman [:rnewman]]

Hi Anita!

To start, set up a build environment - you can see the instructions here:
https://wiki.mozilla.org/Mobile/Fennec/Android

Then, you'll need to create a patch to upload - see
https://wiki.mozilla.org/Mobile/Fennec/Android#Creating_commits_and_submitting_patches

You'll need to familiarize yourself with how Clear Private Data works in Fennec: we send an event from the Java UI to Gecko here:

https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/preferences/PrivateDataPreference.java#58

then we have a static instance of GlobalHistory:

https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GlobalHistory.java


We want to clear the visited set:

https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GlobalHistory.java#46

whenever the user requests to clear their history.

Come find us on irc.mozilla.org, #mobile, or let us know here, if you have questions!

Comment 3

[Anita]

Hi, I would like to add a test - where should I locate it?

Comment 4

[zmiller12]

I figured I would take a look at this one. Could it be as simple as calling 

GlobalHistory.getInstance().mVisitedCache.clear() 

just below   

GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Sanitize:ClearData", json.toString()));

in PrivateDataPreference ?

Comment 5

[zmiller12]

Attachment: Clearing GlobalHistory VisitedCache in PrivateDataPreference

Comment 6

[zmiller12]

Attachment: Clearing GlobalHistory VisitedCache in PrivateDataPreference

Comment 7

[Anita]

Attachment: MozReview Request: Bug 1197869 - Clearing GlobalHistory cache after clearing history from Clear Private Data; r?rnewman

Review commit: https://reviewboard.mozilla.org/r/34977/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34977/

Comment 8

[Anita]

I also would like to commit an unit test but I don't know where is the best place to locate it.

Comment 9

[Richard Newman [:rnewman]]

Sorry for the delayed response, Anita; both of the mentors on this bug are enjoying a holiday weekend in the US.

I'll give you some feedback and a pointer for the test in a moment.

Comment 10

[Richard Newman [:rnewman]]

(In reply to Anita from comment #8)
> I also would like to commit an unit test but I don't know where is the best
> place to locate it.

In this case you probably want to add a Robocop test, which you'll do here:

/mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests

You can look at testOSLocale.java and testEventDispatcher.java as sources of inspiration.

Comment 11

[Richard Newman [:rnewman]]

Comment on attachment 8719487 [details]
MozReview Request: Bug 1197869 - Clearing GlobalHistory cache after clearing history from Clear Private Data; r?rnewman

https://reviewboard.mozilla.org/r/34977/#review31551

::: mobile/android/base/java/org/mozilla/gecko/GeckoAppShell.java:290
(Diff revision 1)
> +    public static void clearPrivateData(String data){

We prefer not to add things to `GeckoAppShell` if we can avoid it -- it's the entry point for calls via JNI, which we don't need here.

You're not wrong in adding a method here -- it encapsulates to make sure you can't clear private data without also having the opportunity to clear GlobalHistory.

But there's a little more elegant way to do this, and there's a nuance that this linkage doesn't address.

The nuance is that not all `ClearData` operations intend to clear history. There's no point in blowing away the cache if we're not actually deleting history.

You can see the way we construct a JSON object in `PrivateDataPreference.java` to specify which boxes are checked. To do this right we need to look in that object.

But!

There is a more elegant way to do this that happens to take care of that nuance.

Messages are passed over the bridge, including `Sanitize:ClearData` (Java to Gecko), `Sanitize:ClearHistory` (Gecko to Java), and `Sanitize:Finished` (Gecko to Java).

(These messages tie in to the Sanitizer on the Gecko side. See modules/Sanitizer.jsm.)

We can listen for those messages in `GlobalHistory`, just as `BrowserApp` does. So it _should_ work to have `GlobalHistory` listen for `Sanitize:ClearHistory`, just like `BrowserApp`, and drop the cache in response to that message.

That's elegant because no matter how `ClearHistory` is triggered -- Clear Private Data, or some action in Gecko -- we'll end up dropping the cache.

To test this, you'd send `Sanitize:ClearHistory`, and make sure that the right thing happens. Easier than clicking around in the UI via test code.

::: mobile/android/base/java/org/mozilla/gecko/GlobalHistory.java:161
(Diff revision 1)
> +        mVisitedCache.clear();

`mVisitedCache` is a `SoftReference`, so you need to dereference it and also check whether it points to `null`. Here you'd need to do this:

```
Set<String> cache = this.mVisitedCache.get();
if (cache != null) {
    cache.clear();
}
```

Comment 12

[Richard Newman [:rnewman]]

(In reply to zmiller12 from comment #4)
> I figured I would take a look at this one.

Hi! Anita is already working on this bug; maybe go take a look at

http://www.joshmatthews.net/bugsahoy/?mobileandroid=1

to find another one for yourself?

Comment 13

[Anita]

Hi, I faced a problem - GlobalHistory is package-private and it is in package org.mozilla.gecko, so I can't access it from org.mozilla.gecko.tests. What should I do in this case?

Comment 14

[Anita]

Attachment: MozReview Request: Bug 1197869 - Clearing GlobalHistory cache after Sanitize:ClearHistory message; r?rnewman

Review commit: https://reviewboard.mozilla.org/r/35615/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35615/

Comment 15

[Anita]

I've changed GlobalHistory to public temporarily for tests reasons - on IRC I've got an answer that a reviewer will suggest a proper solution

Comment 16

[Richard Newman [:rnewman]]

Comment on attachment 8721246 [details]
MozReview Request: Bug 1197869 - Clearing GlobalHistory cache after Sanitize:ClearHistory message; r?rnewman

https://reviewboard.mozilla.org/r/35615/#review32555

This is looking good!

Note that you'll need to annotate any methods that you call from Robocop with @RobocopTarget, otherwise they might be removed by the optimizer and your tests will fail.

Please also squash down these commits. More comments inline.

::: mobile/android/base/java/org/mozilla/gecko/GlobalHistory.java:27
(Diff revision 1)
> -class GlobalHistory {
> +public class GlobalHistory implements NativeEventListener{

Nit: space between interface name and '{'.

::: mobile/android/base/java/org/mozilla/gecko/GlobalHistory.java:49
(Diff revision 1)
> -    SoftReference<Set<String>> mVisitedCache;   // cache of the visited URI list
> +    public SoftReference<Set<String>> mVisitedCache;   // cache of the visited URI list

Rather than making this public, I'd rather have a public method: `cacheIsEmpty`.

::: mobile/android/base/java/org/mozilla/gecko/GlobalHistory.java:165
(Diff revision 1)
> -    public void clear(){
> +      @Override

Nit: make indenting match.

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testClearGlobalHistory.java:45
(Diff revision 1)
> +        GlobalHistory.getInstance().mVisitedCache = new SoftReference<>(visitedSet);

Rather than doing this -- which potentially invalidates the thing you're testing -- try to simulate a real recording of history.

You can do that by navigating to a page within your test page -- indeed, you might find that loading the test page itself adds something to history.

Comment 17

[Richard Newman [:rnewman]]

(In reply to Anita from comment #15)
> I've changed GlobalHistory to public temporarily for tests reasons - on IRC
> I've got an answer that a reviewer will suggest a proper solution

I don't see too much of a problem with it being public.

Comment 18

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.