Closed Bug 1008620 Opened 10 years ago Closed 10 years ago

Clearing history should also clear jump list cache on Windows

Categories

(Core :: Widget: Win32, defect)

16 Branch
x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: bbondy, Assigned: ashutoshd, Mentored)

References

Details

(Keywords: privacy, Whiteboard: [good first bug][lang=c++, js])

Attachments

(1 file, 6 obsolete files)

If a user clears their history, the jumplistCache still contains images of the sites that were visited.
Summary: Clearing history should also clear jump list cache on windows. → Clearing history should also clear jump list cache on Windows
Keywords: qawanted
Any idea how long it will take to resolve this issue?
It shouldn't be too long but I'm currently working on some other things that are high priority.  If someone else has time to join in in the meantime, feel free :)

Bug 724423  did get fixed and landed recently though so that was the biggest part of this.
Mentor: jmathies, tabraldes, netzen
Whiteboard: [good first bug][lang=c++, js]
Assignee: netzen → nobody
I'm interested in patching this bug.Please help me understand this bug more deeply and provide assistance to work on it as I'm new to this.
@ manu.jain: Since there doesn't appear to be any code to 'clear jump list cache' that code would have to be written. I don't know what level of proficiency you are at. You would need to know how to code C++ probably, perhaps some Javascript too. Downloading of the relevant source code would be necessary.

I don't know the details about what compiler / IDE would be necessary or best to use.

/ No expert myself.
I know some c++. Can you tell me how to go ahead with this bug as this is my first bug.
You can needinfo one of the mentors! :)
Hi manu,

I added you as the assignee for now, thanks for offering to help.

The code for jump lists can be found here:
http://dxr.mozilla.org/mozilla-central/search?q=JumpListCache&redirect=true

In particular search for AsyncDeleteAllFaviconsFromDisk.
We'll probably want to add a function call to the jump list interface and then call into that when the cache is cleared.

Use the same dxr search link above to search for the UI text around where clearing cache happens to see what part of that code needs to trigger the call.

Here's the link for the IDL file:
http://dxr.mozilla.org/mozilla-central/source/widget/nsIJumpListBuilder.idl
Contrast that to the definitions found in the CPP linked here:
http://dxr.mozilla.org/mozilla-central/source/widget/windows/JumpListBuilder.cpp

Hopefully that helps a bit, but if you get stuck please feel free to ask again.
Or if you decide to work on something different, just let me know and I'll unassign you.

Thanks again for offering to help and good luck!
Assignee: nobody → manu.jain13
Sorry, I was working on some other bug for 2-3 days. Actually I'm new to this and that was my first bug so it took some time. Now, I can work on this bug. Thanks!!
Status: NEW → ASSIGNED
Attached patch Patch (obsolete) — Splinter Review
Hi Brian,
As you suggested in Comment 7, the code from `AsyncDeleteAllFaviconsFromDisk` gets executed in this patch on clearing history but none of icons in `jumpListCache` directory gets deleted. Please have a look.
Attachment #8485590 - Flags: feedback?(netzen)
Comment on attachment 8485590 [details] [diff] [review]
Patch

Review of attachment 8485590 [details] [diff] [review]:
-----------------------------------------------------------------

Great work, a couple of changes and I think it'll be good to go.

::: browser/components/preferences/in-content/privacy.js
@@ +563,5 @@
> +	// Delete all JumpListCache icons.
> +	var taskbar = Cc["@mozilla.org/windows-taskbar;1"]
> +                .getService(Ci.nsIWinTaskbar);
> +	var builder = taskbar.createJumpListBuilder();
> +	builder.deleteActiveList();

This code actually runs on other non-windows platforms too. 

Please add 
#ifdef XP_WIN
...
#endif

Around this block so that this code only runs on Windows.

::: widget/windows/JumpListBuilder.cpp
@@ +439,5 @@
>  {
> +
> +  // Delete all JumpListCache icons.
> +  nsCOMPtr<nsIRunnable> event =
> +    new mozilla::widget::AsyncDeleteAllFaviconsFromDisk(true);

This should be false passed here so it doesn't ignore deleting the recent icons.

::: widget/windows/WinUtils.cpp
@@ -1342,5 @@
>        // We found an ICO file that exists, so we should remove it
>        currFile->Remove(false);
>      }
>    } while(true);
> -

nit: remove newline added in this file so it doesn't show up in the diff.
Attachment #8485590 - Flags: feedback?(netzen) → feedback+
Thanks Brian.
I have improved the patch as per your comments. Please have a look.
Also, are there any tests to be modified after this? I tried to search but found nothing.
Attachment #8485590 - Attachment is obsolete: true
Attachment #8485863 - Flags: feedback?(netzen)
Comment on attachment 8485863 [details] [diff] [review]
Improved patch as per Comment 10.

Review of attachment 8485863 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/preferences/in-content/privacy.js
@@ +563,5 @@
> +
> +#ifdef XP_WIN
> +	// Delete all JumpListCache icons.
> +	var taskbar = Cc["@mozilla.org/windows-taskbar;1"]
> +                .getService(Ci.nsIWinTaskbar);

Sorry about not catching this right away but I think that creating another instance could have some problems. And it would create another thread.

I think it would be best to from privacy.js call:
observerService.notifyObservers on a new string clear-private-data (You wouldn't need the XP_WIN checks anymore).

And then AddObserver similar to:
http://dxr.mozilla.org/mozilla-central/source/widget/windows/JumpListBuilder.cpp#65

And handle it in JumpListBuilder::Observe.

The handling in JumpListBuilder::Observe would just call this directly:
// Delete JumpListCache icons from disk, if any.
nsCOMPtr<nsIRunnable> event = new mozilla::widget::AsyncDeleteAllFaviconsFromDisk(false);
mIOThread->Dispatch(event, NS_DISPATCH_NORMAL);
Attachment #8485863 - Flags: feedback?(netzen)
Attachment #8485863 - Attachment is obsolete: true
Attachment #8488052 - Flags: feedback?(netzen)
Brian: There seems to be some indentation problems in last patch. I will fix them in the next one.
Comment on attachment 8488052 [details] [diff] [review]
Improved patch as per Comment 12.

Review of attachment 8488052 [details] [diff] [review]:
-----------------------------------------------------------------

This looks correct to me!
Please fix the whitespacing and submit a new patch, I'll do some final testing on my end here and then I'll help you land the patch if you don't have push access yet.

Great work and thank you for the help!

::: browser/components/preferences/in-content/privacy.js
@@ +559,5 @@
>  
>      // reset the timeSpan pref
>      if (aClearEverything)
>        ts.value = timeSpanOrig;
> +	Services.obs.notifyObservers(null, "clear-private-data", null);

nit: I think this should be aligned alongside the if (

::: widget/windows/JumpListBuilder.cpp
@@ +63,5 @@
>    nsCOMPtr<nsIObserverService> observerService =
>      do_GetService("@mozilla.org/observer-service;1");
>    if (observerService) {
>      observerService->AddObserver(this, TOPIC_PROFILE_BEFORE_CHANGE, false);
> +	observerService->AddObserver(this, TOPIC_CLEAR_PRIVATE_DATA, false);

nit: this should be aligned alongside the other observerService->AddObserver

@@ +540,5 @@
>        mIOThread->Dispatch(event, NS_DISPATCH_NORMAL);
>      }
> +  } else if (strcmp(aTopic, TOPIC_CLEAR_PRIVATE_DATA) == 0) {
> +    // Delete JumpListCache icons from Disk, if any.
> +	nsCOMPtr<nsIRunnable> event = 

nit: This should be aligned alongside the comment.
Attachment #8488052 - Flags: feedback?(netzen) → review+
Thanks Brian. Its all your help and support.
I fixed the indentation in the last patch. Please have a look.
Attachment #8488052 - Attachment is obsolete: true
Attachment #8488732 - Flags: feedback?(netzen)
Wait. Last patch still had some indentation problem. This one is correct.
Attachment #8488732 - Attachment is obsolete: true
Attachment #8488732 - Flags: feedback?(netzen)
Attachment #8488736 - Flags: feedback?(netzen)
Attachment #8488736 - Flags: feedback?(netzen) → review+
Shouldn't 

> Services.obs.notifyObservers(null, "clear-private-data", null);

also be added to browser/components/preferences/privacy.js? Right now it seems this would only work in in-content prefs.
(In reply to Mark Straver from comment #18)
> Shouldn't 
> 
> > Services.obs.notifyObservers(null, "clear-private-data", null);
> 

Sorry shouldn't what?

> also be added to browser/components/preferences/privacy.js? Right now it
> seems this would only work in in-content prefs.

Are we still supporting non in-content prefs for FF35?
> Shouldn't
... it was one sentence. I'll try to word myself more carefully next time.

(In reply to Brian R. Bondy [:bbondy] from comment #19)
> Are we still supporting non in-content prefs for FF35?

You're removing the regular preferences window? Heh. Well if it's already removed then indeed it shouldn't be in the patch; I do suggest this gets uplifted though since it's quite an important privacy issue - in which case you'd need it in XUL prefs as well.
Comment on attachment 8488736 [details] [diff] [review]
Patch 8488052 with proper indentation.

OK I Just checked with Jared who authored in content preferences, and although that's the default in Nightly, it will likely not ship with v35. 

So please apply the same fix for preferences for the out of content preferences.
To test that go to about:config and set:
browser.preferences.inContent;false
Attachment #8488736 - Flags: review+
Attached patch Non in-content handle. (obsolete) — Splinter Review
Attachment #8488736 - Attachment is obsolete: true
Attachment #8490171 - Flags: feedback?(netzen)
Comment on attachment 8490171 [details] [diff] [review]
Non in-content handle.

Review of attachment 8490171 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, thanks for taking this and sorry for the delay.
Will post the `try` link shortly.
Attachment #8490171 - Flags: feedback?(netzen) → review+
Assignee: manu.jain13 → ashutoshdhundhara
Added patch headers.
Attachment #8490171 - Attachment is obsolete: true
Attachment #8494176 - Flags: review+
Thanks for all the work Ashutosh!
https://hg.mozilla.org/mozilla-central/rev/426990dc2850
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
I think this patch has a mistake in it.

Correct me if I'm wrong, but the fix for the standard XUL preferences window seems to put this under clearing the web cache and the offline web app data functions, and not under privacy -> clear history...?

Shouldn't this, instead, be in clearPrivateDataNow in browser/components/preferences/privacy.js just like is done for the in-content prefs?
Flags: needinfo?(ashutoshdhundhara)
(In reply to Mark Straver from comment #29)

Hi Mark,

> I think this patch has a mistake in it.
> 
> Correct me if I'm wrong, but the fix for the standard XUL preferences window
> seems to put this under clearing the web cache and the offline web app data
> functions, and not under privacy -> clear history...?

I tested this fix for the standard XUL preferences window for `clear your recent history` link under `Privacy` tab and it works fine.

> 
> Shouldn't this, instead, be in clearPrivateDataNow in
> browser/components/preferences/privacy.js just like is done for the
> in-content prefs?

I think doing the same we did for in-content prefs wouldn't work here as the option for clearing the private data is also available under `Advanced` tab. So we would need to modify the corresponding functions in `advanced.js` as well.

Brian, can you confirm if I am getting this correct or not?

Thanks!
Flags: needinfo?(ashutoshdhundhara)
(In reply to Ashutosh Dhundhara [:ashutoshd] from comment #30)
> I tested this fix for the standard XUL preferences window for `clear your
> recent history` link under `Privacy` tab and it works fine.
Really? Even if you not check "cache" there? I think it would only be cleared right now if and when you clear your cache or offline web app data, not when you clear your browsing history (that this function should be triggered by).

Of course the -default- setting is to clear your cache as well when clearing history, but that's certainly not a given.

> I think doing the same we did for in-content prefs wouldn't work here as the
> option for clearing the private data is also available under `Advanced` tab.

Um, no, it isn't available there. The advanced category doesn't have the option to clear your history. It has the option to clear cached web data or cached app/site specific stored data, but not history. I think you might have mixed up cache and private data (like history and forms) here ;)

Conversely, if you expect the "advanced" options for XUL preferences to be the correct place, then it should also be in those locations for in-content preferences. It can't really be so that XUL and in-content prefs work essentially differently for the same controls. They are pretty much a 1:1 mirror (or they should be, at least)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: