Closed Bug 1211849 Opened 7 years ago Closed 7 years ago

Remove canClear support from the Sanitizer

Categories

(Firefox :: General, defect)

Product:
Firefox
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 46
Tracking Flags:
Tracking Status
firefox46 --- verified

People

(Reporter: mak, Assigned: nwokop, Mentored)

References

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(2 files, 3 obsolete files)

more test cleanup
52.08 KB, patch
ritu
: approval-mozilla-beta+
Details | Diff | Splinter Review
BETA rebased patch
52.66 KB, patch
Details | Diff | Splinter Review 
we will default to true and let single components deal with no-ops.

See https://bugzilla.mozilla.org/show_bug.cgi?id=474431#c42 and following comments for the reasoning.
Whiteboard: [good first bug][lang=js]
Attached patch Initial patch (obsolete) — — Splinter Review
Attachment #8670898 - Flags: review+
In my patch I deleted all uses of canClear functions or vars everywhere I found but I’m sure it’s the way to go… Anyway it works according to my tests.
*I’m not sure it’s the way to go, because I don’t know the functions were used outside of sanitizeDialog.js.
After a find & grep:

# bug 409624

- browser/base/content/test/general/browser_bug409624.js: canClear() for formdata is used… I may move only the code which checks search bar emptiness in the test.
- toolkit/content/tests/chrome/bug409624_window.xul: I may delete a few lines here.
- toolkit/content/widgets/findbar.xml: I may delete the canClear property.

# Other

- mobile/android/modules/Sanitizer.jsm: canClear() for syncedTabs contains code but it seems it’s duplicated in clear() so that may be removed safely? Others canClear() only return true.
- dom/settings/SettingsRequestManager.jsm: canClear property seems unrelated to Sanitizer.

What do you think?
Flags: needinfo?(mak77)
Comment on attachment 8670898 [details] [diff] [review]
Initial patch

I suppose you didn't mean to set review+ on the patch, since it's not yet reviewed :)
Flags: needinfo?(mak77)
Attachment #8670898 - Flags: review+
(In reply to ariasuni from comment #4)
> - browser/base/content/test/general/browser_bug409624.js: canClear() for
> formdata is used… I may move only the code which checks search bar emptiness
> in the test.
> - toolkit/content/tests/chrome/bug409624_window.xul: I may delete a few
> lines here.
> - toolkit/content/widgets/findbar.xml: I may delete the canClear property.

I will look into these and let you know in a next comment.

> # Other
> - mobile/android/modules/Sanitizer.jsm: canClear() for syncedTabs contains
> code but it seems it’s duplicated in clear() so that may be removed safely?
> Others canClear() only return true.
> - dom/settings/SettingsRequestManager.jsm: canClear property seems unrelated
> to Sanitizer.

Please don't touch mobile code, it's a completely separate codebase from Firefox Desktop, this bug is only about the desktop code.
Comment on attachment 8670898 [details] [diff] [review]
Initial patch

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

(In reply to ariasuni from comment #4)
> - toolkit/content/widgets/findbar.xml: I may delete the canClear property.

yes, this makes sense, it's only used for canClear...

> - browser/base/content/test/general/browser_bug409624.js: canClear() for
> formdata is used… I may move only the code which checks search bar emptiness
> in the test.

you could copy the code from findbar.xml to the test, put it into a findbarHasData() helper that returns whether the findbar has data (note from the test you can use gFindBar._findField)
In the end the helper will check the value and the transaction manager contents.

> - toolkit/content/tests/chrome/bug409624_window.xul: I may delete a few
> lines here.

I'd rather do the same as browser_bug409624.js, in the end we'll just replace a method with an helper function...

> - mobile/android/modules/Sanitizer.jsm: canClear() for syncedTabs contains
> code but it seems it’s duplicated in clear() so that may be removed safely?
> Others canClear() only return true.

Please don't touch mobile code.

> - dom/settings/SettingsRequestManager.jsm: canClear property seems unrelated
> to Sanitizer.

yes, it's unrelated and should not be touched.

::: browser/base/content/sanitizeDialog.js
@@ -74,5 @@
>      else
>        this.warningBox.hidden = true;
> -
> -    Promise.all(tasks).then(() => {
> -      Services.obs.notifyObservers(null, "sanitize-dialog-setup-complete", "");

since you don't notify this anymore (and it looks ok since now there's no more async initialization), you can remove the only consumer of this topic here
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_sanitizeDialog.js#914
that means you can remove promiseSanitizationDialogReady and promiseDialogReady
Attachment #8670898 - Flags: feedback+
Hi, any news about this?
Flags: needinfo?(perso+bugmoz)
Blocks: 1167238
Sorry, I was busy. I’m working on it right now.

About browser_bug409624.js and bug409624_window.xul: can I create a function with the code of findbar.xml in one of those and use it in the other, or do I need to copy the code in both files?
Flags: needinfo?(perso+bugmoz)
(In reply to ariasuni from comment #9)
> Sorry, I was busy. I’m working on it right now.
> 
> About browser_bug409624.js and bug409624_window.xul: can I create a function
> with the code of findbar.xml in one of those and use it in the other, or do
> I need to copy the code in both files?

You need to copy the code into both files, cause one is a mochitest-browser test, the other one is a mochitest-chrome test. one is in browser, the other one in toolkit, they don't share much utils at the moment (it's ideally possible to create a module and expose it through testing-common resource, but it's not worth the effort here)
Any problems I can help with?
Sorry if I am nagging you, the fact is this is soft-blocking bug 1167238 and I'd like to unblock it.
I'd like to work on this one.
I might admit I’m don’t really know how to hack on browser/base/content/test/general/browser_bug409624.js, I don’t understand the code enough.

Otherwise, I think It’s okay.
(In reply to ariasuni from comment #13)
> I might admit I’m don’t really know how to hack on
> browser/base/content/test/general/browser_bug409624.js, I don’t understand
> the code enough.
> 
> Otherwise, I think It’s okay.

you should just create a findBarHasData() function in the global object of the test,  then replace the calls to s.canClearItem("formdata", callback, s); with simple ok() checks of findBarHasData() (copying the current aResult checks)
findBarHasData should just contain the code the currently is in findBar.xml canClear() with the difference that "this" will be gFindBar

The same for bug409624_window.xul

both tests can be run using ./mach mochitest relative/path/to/the/test
(In reply to Alex Johnson(:alex_johnson) from comment #12)
> I'd like to work on this one.

Sorry! Please ignore this comment. I did not realize that someone was working on it.
well, if ariasuni thinks to not have enough time to complete it, he can probably pass the bug to you. let's see how it goes.
Can I juste keep/rename the function in findbar.xml and use it directly in browser_bug409624.js and bug409624_window.xul? Isn’t that easier?
yes, you could rename it too, if that helps.
I didn't suggest that cause usually we don't keep around APIs just for tests, but it's a minor thing and shouldn't have bad effects.
If you don't plan on completing this, or you don't have the time to do that, please leave the bug to Alex, so we can proceed.
Flags: needinfo?(perso+bugmoz)
Well even before I edit anything, ./mach mochitest browser/base/content/test/general/browser_bug409624.js fails with a timeout so I can’t test my changes… Maybe I should let someone else finish it after all.
Flags: needinfo?(perso+bugmoz)
I would be happy to complete this one if it's ok with you.
(In reply to Patrick from comment #21)
> I would be happy to complete this one if it's ok with you.

I'd be OK with that.  I will be busy with Fennec stuff until next week anyway.
Assignee: nobody → nwokop
Status: NEW → ASSIGNED
(In reply to Patrick from comment #21)
> I would be happy to complete this one if it's ok with you.

Sounds good, if you still have issues with the above test (Comment 20) please let me know and I'll see what I can figure out.
Thanks Marco.

I am still not able to run the test because because it keeps timing out.
Attached patch Bug_1211849.diff - Rough Patch (obsolete) — — Splinter Review 
Can you please advice if I'm on the right path here.
Attachment #8697682 - Flags: review?(mak77)
Attached patch final patch (obsolete) — — Splinter Review 
thanks everyone, I have merged the patches and cleaned up the test. Locally it seems to be working fine, I don't see timeouts. Let's see what Try thinks about it.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6bfe62fe39f
Attachment #8670898 - Attachment is obsolete: true
Attachment #8697682 - Attachment is obsolete: true
Attachment #8697682 - Flags: review?(mak77)
Flags: needinfo?(mak77)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e73675d74d64
Attachment #8701075 - Attachment is obsolete: true
Flags: needinfo?(mak77)
https://hg.mozilla.org/mozilla-central/rev/ea9e749bb86b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Looks like this has increased the average time taken to clear form data:
http://alerts.telemetry.mozilla.org/index.html#/detectors/1/metrics/1130/alerts/?from=2015-12-26&to=2015-12-26

I think this might be explained by the fact that the canClear implementation (which was previously usually always invoked before clear(), given how the UI worked) could have been "priming the cache" such that later accesses of the form history database were faster. In which case this might not necessarily be considered a "real" regression.

On the other hand, it seems to have been a net improvement in clearing times for Downloads:

http://alerts.telemetry.mozilla.org/index.html#/detectors/1/metrics/1127/alerts/?from=2015-12-26&to=2015-12-26

I can't think of any theory that would explain this one.

(I was a bit confused about why this didn't get an explicit r+ in the bug, too. I suppose that is just an oversight from comment 26.)
Flags: needinfo?(mak77)
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #30)
> Looks like this has increased the average time taken to clear form data:
> http://alerts.telemetry.mozilla.org/index.html#/detectors/1/metrics/1130/
> alerts/?from=2015-12-26&to=2015-12-26

This is interesting. As you said it's likely just a time shift, we were spending the same time time before showing the dialog, and saving it later when clearing. I guess we may investigate ways to shortcut clearing an empty form history, it would indeed make more sense for most consumers to have a fast path for clearing empty datasets than to have a canClear api.

I will file a bug to investigate whether we can shortcut the empty dataset case.

> On the other hand, it seems to have been a net improvement in clearing times
> for Downloads:
> 
> http://alerts.telemetry.mozilla.org/index.html#/detectors/1/metrics/1127/
> alerts/?from=2015-12-26&to=2015-12-26
> 
> I can't think of any theory that would explain this one.

May be just a time skew due to different disk usage (related to other IO operations like form history new timing), since canClear was a no-op on downloads.

> 
> (I was a bit confused about why this didn't get an explicit r+ in the bug,
> too. I suppose that is just an oversight from comment 26.)

Yeah, basically my review was in form of a patch, since this bug was blocking further work and the the remaining pieces were trivial.
Flags: needinfo?(mak77)
filed bug 1236455, thank you for the heads up.
Depends on: 1244650
Comment on attachment 8701532 [details] [diff] [review]
more test cleanup

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: wee need to uplift this, to be able to uplift bug 1244650, otherwise the patch is too hard and risky to rebase
[Describe test coverage new/current, TreeHerder]: nightly/aurora
[Risks and why]: This is mostly code removal and some test changes, so risk is limited
[String/UUID change made/needed]: none
Attachment #8701532 - Flags: approval-mozilla-beta?
Comment on attachment 8701532 [details] [diff] [review]
more test cleanup

Taking this in Beta45 as we need it to be able to uplift fix from bug 1244650 to m-b.
Attachment #8701532 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name 			Firefox
Version 		46.0b4
Build ID 		20160322075646
Update Channel 	        beta
User Agent 		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS                      Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing.

Actual Results: 
As expected
I can verify this by code inspection.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.