1479445 - PlacesUtils.history.update shouldn't require the url to be specified when the guid is (update validation handling)

Status: RESOLVED FIXED

(Toolkit :: Places, enhancement, P3)

Description

[Mark Banner (:standard8)]

Currently PlacesUtils.history.update requires a `url` option even when `guid` is supplied.

It should ideally be updated to allow supplying either the url or the guid, and the existing code updated to do the same.

One possible way to do this, would be to rewrite the validation routines (validatePageInfo) to work in the same way as the bookmark validation routines.

Comment 1

[Siddhant [:Siddhant085]]

validItemProperties accepts the properties to be verified and also the function calls to do the verification. We can use this itself to validatePageInfo. A few changes can be made to accommodate verification of either of the properties (url or guid). All the calls being made to validatePageInfo will have to be changed.

Comment 2

[Siddhant [:Siddhant085]]

Ok we don't require to make changes to validateItemProperties. As analysed how bookmark validation is being handled. We can declare PAGEINFO_VALIDATORS, but it will be duplication of code to some extent as both bookmark and PageInfo has overlapping properties. We can declare PAGEINFO_VALIDATORS to use validation functions from BOOKMARK_VALIDATORS. validatePageInfo can set what is required and what is optional and call validateItemProperties.

Comment 3

[Siddhant [:Siddhant085]]

I defined PAGEINFO_VALIDATORS and called validateItemProperties from inside validatePageInfo. Test cases are failing because the type and the message of errors thrown are different (more general). Should I change the test cases?

Comment 4

[Mark Banner (:standard8)]

(In reply to Siddhant [:Siddhant085] from comment #2)
> Ok we don't require to make changes to validateItemProperties. As analysed
> how bookmark validation is being handled. We can declare
> PAGEINFO_VALIDATORS, but it will be duplication of code to some extent as
> both bookmark and PageInfo has overlapping properties. We can declare
> PAGEINFO_VALIDATORS to use validation functions from BOOKMARK_VALIDATORS.
> validatePageInfo can set what is required and what is optional and call
> validateItemProperties.

I think that sounds fine, sharing validation functions is a good thing to do - there will be some overlap, we can't do much about that.

(In reply to Siddhant [:Siddhant085] from comment #3)
> I defined PAGEINFO_VALIDATORS and called validateItemProperties from inside
> validatePageInfo. Test cases are failing because the type and the message of
> errors thrown are different (more general). Should I change the test cases?

Generally that's fine, as long as the functional aspects of the code are still running correctly. Changing the tests should be fine. You might want to just have a look at some of the callers to check they aren't relying on the error types, but I'm pretty sure we don't generally.

Comment 5

[Siddhant [:Siddhant085]]

Attachment: Bug 1479445: Update the validation of PageInfo to use validateItemProperties

Changed the validation function for PageInfo to use a more general validateItemProperties.
This changes the error message being thrown. Changed the respective test cases to accomodate the change.

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 9008976 [details]
Bug 1479445: Update the validation of PageInfo to use validateItemProperties

Mark Banner (:standard8) has approved the revision.

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 9008976 [details]
Bug 1479445: Update the validation of PageInfo to use validateItemProperties

Marco Bonardo [::mak] has approved the revision.

Comment 8

[Natalia Csoregi [:nataliaCs]]

Backed out changeset for failures on /xpcshell/test_ext_history.js

backout: https://hg.mozilla.org/integration/autoland/rev/28bdfb2c69a91ab8b4cb0080a0b8487f7f334c8b

push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=xpcshell&group_state=expanded&revision=db16547e0d608eb51fc876fcbb43c23c071d6f63

failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=201251357&repo=autoland&lineNumber=2121

[task 2018-09-24T18:47:46.763Z] 18:47:46     INFO -  TEST-START | xpcshell-remote.ini:browser/components/extensions/test/xpcshell/test_ext_history.js
[task 2018-09-24T18:47:51.232Z] 18:47:51  WARNING -  TEST-UNEXPECTED-FAIL | xpcshell-remote.ini:browser/components/extensions/test/xpcshell/test_ext_history.js | xpcshell return code: 0
[task 2018-09-24T18:47:51.232Z] 18:47:51     INFO -  TEST-INFO took 4464ms
[task 2018-09-24T18:47:51.233Z] 18:47:51     INFO -  >>>>>>>
[task 2018-09-24T18:47:51.233Z] 18:47:51     INFO -  PID 12530 | [12530, Main Thread] WARNING: Couldn't get the user appdata directory. Crash events may not be produced.: file /builds/worker/workspace/build/src/toolkit/crashreporter/nsExceptionHandler.cpp, line 2723
[task 2018-09-24T18:47:51.233Z] 18:47:51     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2018-09-24T18:47:51.233Z] 18:47:51     INFO -  (xpcshell/head.js) | test run_next_test 0 pending (2)
[task 2018-09-24T18:47:51.233Z] 18:47:51     INFO -  (xpcshell/head.js) | test MAIN run_test finished (2)
[task 2018-09-24T18:47:51.234Z] 18:47:51     INFO -  running event loop
[task 2018-09-24T18:47:51.236Z] 18:47:51     INFO -  xpcshell-remote.ini:browser/components/extensions/test/xpcshell/test_ext_history.js | Starting test_delete
[task 2018-09-24T18:47:51.238Z] 18:47:51     INFO -  (xpcshell/head.js) | test test_delete pending (2)
[task 2018-09-24T18:47:51.239Z] 18:47:51     INFO -  "Extension attached"
[task 2018-09-24T18:47:51.241Z] 18:47:51     INFO -  PID 12530 | [12530, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80040111: file /builds/worker/workspace/build/src/netwerk/protocol/res/SubstitutingProtocolHandler.cpp, line 342
[task 2018-09-24T18:47:51.243Z] 18:47:51     INFO -  (xpcshell/head.js) | test run_next_test 0 finished (2)
[task 2018-09-24T18:47:51.248Z] 18:47:51     INFO -  PID 12530 | [12530, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004002: file /builds/worker/workspace/build/src/toolkit/components/resistfingerprinting/nsRFPService.cpp, line 683
[task 2018-09-24T18:47:51.250Z] 18:47:51     INFO -  PID 12530 | [12530, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520012: file /builds/worker/workspace/build/src/extensions/cookie/nsPermissionManager.cpp, line 2932
[task 2018-09-24T18:47:51.255Z] 18:47:51     INFO -  PID 12530 | [12530, Main Thread] WARNING: GLX_swap_control unsupported, ASAP mode may still block on buffer swaps.: file /builds/worker/workspace/build/src/gfx/gl/GLContextProviderGLX.cpp, line 226
[task 2018-09-24T18:47:51.257Z] 18:47:51     INFO -  PID 12530 | [12530, Main Thread] WARNING: SGI_video_sync unsupported. Falling back to software vsync.: file /builds/worker/workspace/build/src/gfx/thebes/gfxPlatformGtk.cpp, line 823
[task 2018-09-24T18:47:51.260Z] 18:47:51     INFO -  PID 12530 | ++DOCSHELL 0x7fb61bddf000 == 1 [pid = 12530] [id = {0225b899-a3d2-4a26-871c-1b37781b1378}]
[task 2018-09-24T18:47:51.262Z] 18:47:51     INFO -  PID 12530 | ++DOMWINDOW == 1 (0x7fb612635000) [pid = 12530] [serial = 1] [outer = (nil)]
[task 2018-09-24T18:47:51.264Z] 18:47:51     INFO -  PID 12530 | ++DOMWINDOW == 2 (0x7fb612668000) [pid = 12530] [serial = 2] [outer = 0x7fb612635000]
[task 2018-09-24T18:47:51.266Z] 18:47:51     INFO -  PID 12530 | ++DOMWINDOW == 3 (0x7fb6126ce400) [pid = 12530] [serial = 3] [outer = 0x7fb612635000]
[task 2018-09-24T18:47:51.268Z] 18:47:51     INFO -  PID 12530 | [Parent 12530, Main Thread] WARNING: Couldn't get the user appdata directory, crash dumps will go in an unusual location: file /builds/worker/workspace/build/src/toolkit/crashreporter/nsExceptionHandler.cpp, line 2800
[task 2018-09-24T18:47:51.271Z] 18:47:51     INFO -  PID 12530 | LoadPlugin() /tmp/xpc-plugins-KXk5eZ/libnpsecondtest.so returned 7fb61c43c070
[task 2018-09-24T18:47:51.276Z] 18:47:51     INFO -  PID 12530 | LoadPlugin() /tmp/xpc-plugins-KXk5eZ/libnptest.so returned 7fb61d91b280
[task 2018-09-24T18:47:51.278Z] 18:47:51     INFO -  PID 12530 | LoadPlugin() /tmp/xpc-plugins-KXk5eZ/libnpswftest.so returned 7fb61d91b640
[task 2018-09-24T18:47:51.280Z] 18:47:51     INFO -  PID 12530 | LoadPlugin() /tmp/xpc-plugins-KXk5eZ/libnpthirdtest.so returned 7fb61d91b880
[task 2018-09-24T18:47:51.282Z] 18:47:51     INFO -  PID 12530 | [Parent 12530, Main Thread] WARNING: Need TabChild to get the nativeWindow from!: file /builds/worker/workspace/build/src/widget/PuppetWidget.cpp, line 1187
[task 2018-09-24T18:47:51.284Z] 18:47:51     INFO -  PID 12530 | Couldn't convert chrome URL: chrome://branding/locale/brand.properties
[task 2018-09-24T18:47:51.285Z] 18:47:51     INFO -  PID 12530 | ++DOCSHELL 0x7ff5ba0f6800 == 1 [pid = 12574] [id = {2d0847f3-2c2f-42f0-bbdb-0b083708bd9e}]
[task 2018-09-24T18:47:51.286Z] 18:47:51     INFO -  PID 12530 | ++DOMWINDOW == 1 (0x7ff5ba063a00) [pid = 12574] [serial = 1] [outer = (nil)]
[task 2018-09-24T18:47:51.287Z] 18:47:51     INFO -  PID 12530 | [Child 12574, Main Thread] WARNING: Fallback to BasicLayerManager: file /builds/worker/workspace/build/src/dom/ipc/TabChild.cpp, line 2858
[task 2018-09-24T18:47:51.288Z] 18:47:51     INFO -  PID 12530 | ++DOMWINDOW == 2 (0x7ff5ba159400) [pid = 12574] [serial = 2] [outer = 0x7ff5ba063a00]
[task 2018-09-24T18:47:51.289Z] 18:47:51     INFO -  PID 12530 | JavaScript strict warning: resource://gre/modules/RemoteWebProgress.jsm, line 184: ReferenceError: reference to undefined property "matchedList"
[task 2018-09-24T18:47:51.290Z] 18:47:51     INFO -  "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "ReferenceError: reference to undefined property "matchedList"" {file: "resource://gre/modules/RemoteWebProgress.jsm" line: 184}]"

Comment 9

[Mark Banner (:standard8)]

Siddhant: Looks like we missed updating browser/components/extensions/test/xpcshell/test_ext_history.js for the changes in error messages.

If you can look at fixing that up, when you're ready to push a new patch, you'll need to go into phabricator for the patch, scroll down to the bottom, and select "Reopen Revision" from the "add action" menu, and then hit submit.

Comment 10

[Siddhant [:Siddhant085]]

Missed browser/components/extensions/test/xpcshell/test_ext_history.js. Fixed now.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d4c858c70bc9
Update the validation of PageInfo to use validateItemProperties r=mak,Standard8

Comment 12

[Andreea Pavel [:apavel]]

Backed out for failing xpcshell at services/sync/tests/unit/test_corrupt_keys.js

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=d4c858c70bc9870a88d841ad933b9ea74fe547bc

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=201502476&repo=autoland&lineNumber=10511

Backout: https://hg.mozilla.org/integration/autoland/rev/6a531a9eb798d23014f3fc74a5d9bb42ac893c5b

Comment 13

[Mark Banner (:standard8)]

Siddhant: As mentioned offline, digging found that Sync is passing a special object to the PlacesUtils.history.insertMany() call.

It turns out this is unexpected, and a left-over from a previous refactoring.

The suggestion is that we update _recordToPlaceInfo to return a new object that is pageInfo compatible: https://searchfox.org/mozilla-central/rev/819cd31a93fd50b7167979607371878c4d6f18e8/services/sync/modules/engines/history.js#336-436

Do you think you could do that in a separate changeset, and have the current changeset depend on it?

If you don't want to do that, it is alright, we can do the work to unblock this landing.

Comment 14

[Siddhant [:Siddhant085]]

I am creating and returning a new PageInfo object from _recordToPlaceInfo. The return value is checked and if it is not false it is pushed to the toAdd list. test_corrupt_keys.js is passing. I also ran services/sync/tests/unit, all the tests passed. Should I create a new bug to push it in as a separate change set. Also should I run anymore tests to make sure nothing breaks the next time we land this changeset.

Comment 15

[Siddhant [:Siddhant085]]

Attachment: Bug 1479445 - Return a new PageInfo object from _recordToPageInfo so that validateItemProperties can validate the object.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fa8c1d182709
Update the validation of PageInfo to use validateItemProperties r=mak,Standard8
https://hg.mozilla.org/integration/autoland/rev/aa2bf2716f9b
Return a new PageInfo object from _recordToPageInfo so that validateItemProperties can validate the object. r=lina

Comment 17

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/fa8c1d182709
https://hg.mozilla.org/mozilla-central/rev/aa2bf2716f9b