1347452 - Change insertBookmark to use history.makeGuid

Status: RESOLVED FIXED

(Toolkit :: Places, enhancement, P3)

Description

[Marco Bonardo [:mak]]

The module is currently running a SELECT GENERATE_GUID() AS guid query, it should instead use PlacesUtils.history.makeGuid() in insert(), before the call to validateBookmarkObject.
validateBookmarkObject should then grow a guid: { required: true} property, so we'll also check makeGuid returned a valid value.
No test required, the existing tests should already be covering this:
./mach test toolkit/components/places/tests/bookmarks/

Comment 1

[:prathiksha]

Hi. I'd like to be assigned to solve this bug. :)

Comment 2

[:prathiksha]

Hi Marco, Can you give me the path to the file I should be working on ? :) Thanks!

Comment 3

[Marco Bonardo [:mak]]

http://searchfox.org/mozilla-central/source/toolkit/components/places/Bookmarks.jsm

Comment 4

[Pauline]

Hello, could I be assigned to this bug is Prathiksha is no longer interested in solving it?

Thanks

Comment 5

[Mark Hammond [:markh] [:mhammond]]

(In reply to Pauline from comment #4)
> Hello, could I be assigned to this bug is Prathiksha is no longer interested
> in solving it?

Hi Pauline, thanks for you interest! Usually we assign a bug after a patch has been uploaded, so please feel free to start work on this and upload a first cut at a patch for feedback - at that time we'll assign the bug to you and help you to get it ready for landing.

Comment 6

[Riko Ho]

Hello everybody,

I'm newbie in this firefox development and want to contribute a little bit if possible,
I'm interested with this bug, where should I start to fix it ?
I have installed mozzila-central and ./mach run on my desktop,

How can I see this bug on that code ?
Thanks

Comment 7

[Mark Hammond [:markh] [:mhammond]]

(In reply to Riko Ho from comment #6)
> I'm interested with this bug, where should I start to fix it ?

comment 3 tells you the file that needs to be changed.

> I have installed mozzila-central and ./mach run on my desktop,
> 
> How can I see this bug on that code ?

Note that you can't actually *see* this bug - it is an internal optimization that isn't visible to users (but that's true of many bugs here and doesn't make them any less worthwhile).

So have a look at that file, find the code referenced in comment 0, and if you have further specific questions we are happy to help!

Comment 8

[Riko Ho]

This is what I got from
./mach test toolkit/components/places/tests/bookmarks/
Please guide me how to find "Change insertBookmark to use history.makeGuid" ?
================
 0:00.94 Log will not be kept for this command: [Errno 13] Permission denied: u'/home/bianchi/Documents/FirefoxDev/src/mozilla-central/obj-x86_64-pc-linux-gnu/.mozbuild/last_log.json'.
cat: backend.TestManifestBackend.in: No such file or directory
Build configuration changed. Regenerating backend.
Traceback (most recent call last):
  File "/home/bianchi/Documents/FirefoxDev/src/mozilla-central/build/gen_test_backend.py", line 39, in <module>
    sys.exit(gen_test_backend())
  File "/home/bianchi/Documents/FirefoxDev/src/mozilla-central/build/gen_test_backend.py", line 35, in gen_test_backend
    backend.consume(data)
  File "/home/bianchi/Documents/FirefoxDev/src/mozilla-central/python/mozbuild/mozbuild/backend/base.py", line 143, in consume
    self.consume_finished()
  File "/home/bianchi/Documents/FirefoxDev/src/mozilla-central/python/mozbuild/mozbuild/backend/test_manifest.py", line 59, in consume_finished
    pickle.dump(dict(self.tests_by_path), fh, protocol=2)
  File "/usr/lib/python2.7/contextlib.py", line 24, in __exit__
    self.gen.next()
  File "/home/bianchi/Documents/FirefoxDev/src/mozilla-central/python/mozbuild/mozbuild/backend/base.py", line 236, in _write_file
    existed, updated = fh.close()
  File "/home/bianchi/Documents/FirefoxDev/src/mozilla-central/python/mozbuild/mozbuild/util.py", line 269, in close
    with open(self.name, writemode) as file:
IOError: [Errno 13] Permission denied: u'/home/bianchi/Documents/FirefoxDev/src/mozilla-central/obj-x86_64-pc-linux-gnu/all-tests.pkl'
/home/bianchi/Documents/FirefoxDev/src/mozilla-central/build/rebuild-backend.mk:24: recipe for target 'backend.TestManifestBackend' failed
make: *** [backend.TestManifestBackend] Error 1
Error running mach:

    ['test', 'toolkit/components/places/tests/bookmarks/']

The error occurred in code that was called by the mach command. This is either
a bug in the called code itself or in the way that mach is calling it.

You should consider filing a bug for this issue.

If filing a bug, please include the full output of mach, including this error
message.

The details of the failure are as follows:

Exception: Process executed with non-0 exit code 2: [u'/usr/bin/make', u'-f', u'/home/bianchi/Documents/FirefoxDev/src/mozilla-central/build/rebuild-backend.mk', u'-j8', u'-s', u'backend.TestManifestBackend']

  File "/home/bianchi/Documents/FirefoxDev/src/mozilla-central/testing/mach_commands.py", line 242, in test
    resolver = self._spawn(TestResolver)
  File "/home/bianchi/Documents/FirefoxDev/src/mozilla-central/python/mozbuild/mozbuild/base.py", line 623, in _spawn
    topobjdir=self.topobjdir)
  File "/home/bianchi/Documents/FirefoxDev/src/mozilla-central/python/mozbuild/mozbuild/testing.py", line 195, in __init__
    self.topsrcdir, 'build', 'gen_test_backend.py'),
  File "/home/bianchi/Documents/FirefoxDev/src/mozilla-central/python/mozbuild/mozbuild/base.py", line 551, in _run_make
    return fn(**params)
  File "/home/bianchi/Documents/FirefoxDev/src/mozilla-central/python/mozbuild/mozbuild/base.py", line 606, in _run_command_in_objdir
    return self.run_process(cwd=self.topobjdir, **args)
  File "/home/bianchi/Documents/FirefoxDev/src/mozilla-central/python/mach/mach/mixin/process.py", line 147, in run_process
    raise Exception('Process executed with non-0 exit code %d: %s' % (status, args))
===============

Comment 9

[Mark Hammond [:markh] [:mhammond]]

(In reply to Riko Ho from comment #8)
> This is what I got from
> ./mach test toolkit/components/places/tests/bookmarks/

That's odd -  I can't explain why those "permission denied" errors exist. Did you run "./mach build" first and have it succeed? If you run that command before making any changes, it should work (and doing another "./mach build" then re-running the tests it after you made your changes helps ensure your changes are working correctly).

> Please guide me how to find "Change insertBookmark to use history.makeGuid" ?

As mentioned in comment 0, you need to find where a GUID is being generated by SQL like |SELECT GENERATE_GUID() AS guid| and replace that with a call to PlacesUtils.history.makeGuid(), which is a different but more efficient way of getting a GUID.

Comment 10

[Riko Ho]

I got the file :
mozilla-central/toolkit/components/places$ ls
BookmarkHTMLUtils.jsm

Comment 11

[Riko Ho]

(In reply to Mark Hammond [:markh] from comment #9)
> (In reply to Riko Ho from comment #8)
> > This is what I got from
> > ./mach test toolkit/components/places/tests/bookmarks/
> 
> That's odd -  I can't explain why those "permission denied" errors exist.
> Did you run "./mach build" first and have it succeed? If you run that
> command before making any changes, it should work (and doing another "./mach
> build" then re-running the tests it after you made your changes helps ensure
> your changes are working correctly).
> 
> > Please guide me how to find "Change insertBookmark to use history.makeGuid" ?
> 
> As mentioned in comment 0, you need to find where a GUID is being generated
> by SQL like |SELECT GENERATE_GUID() AS guid| and replace that with a call to
> PlacesUtils.history.makeGuid(), which is a different but more efficient way
> of getting a GUID.

W(In reply to Mark Hammond [:markh] from comment #9)
> (In reply to Riko Ho from comment #8)
> > This is what I got from
> > ./mach test toolkit/components/places/tests/bookmarks/
> 
> That's odd -  I can't explain why those "permission denied" errors exist.
> Did you run "./mach build" first and have it succeed? If you run that
> command before making any changes, it should work (and doing another "./mach
> build" then re-running the tests it after you made your changes helps ensure
> your changes are working correctly).
> 
> > Please guide me how to find "Change insertBookmark to use history.makeGuid" ?
> 
> As mentioned in comment 0, you need to find where a GUID is being generated
> by SQL like |SELECT GENERATE_GUID() AS guid| and replace that with a call to
> PlacesUtils.history.makeGuid(), which is a different but more efficient way
> of getting a GUID.

Ok...Let me have a look...I got the file already, thanks

Comment 12

[Riko Ho]

(In reply to Mark Hammond [:markh] from comment #9)
> (In reply to Riko Ho from comment #8)
> > This is what I got from
> > ./mach test toolkit/components/places/tests/bookmarks/
> 
> That's odd -  I can't explain why those "permission denied" errors exist.
> Did you run "./mach build" first and have it succeed? If you run that
> command before making any changes, it should work (and doing another "./mach
> build" then re-running the tests it after you made your changes helps ensure
> your changes are working correctly).
> 

Yes I run "./mach build" and it had succeeded,
I followed :
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build/Linux_and_MacOS_build_preparation

Comment 13

[Riko Ho]

Is that the one ?

/*****/
Function :
function maybeInsertManyPlaces(db, urls)

Line 2026:
maybeguid: PlacesUtils.history.makeGuid(),

File : Bookmarks.jsm
/******/

I need to find where GUID is ?

 and 
/****/
"./mach test toolkit/components/places/tests/bookmarks/" is the way testing it ?
/****/

Comment 14

[Mark Hammond [:markh] [:mhammond]]

(In reply to Riko Ho from comment #13)
> Is that the one ?
> 
> /*****/
> Function :
> function maybeInsertManyPlaces(db, urls)
> 
> Line 2026:
> maybeguid: PlacesUtils.history.makeGuid(),

Nope - that is somewhere that *already* calls PlacesUtils.history.makeGuid() - in this bug you need to find the SQL statement mentioned above and replace it with PlacesUtils.history.makeGuid().

>  and 
> /****/
> "./mach test toolkit/components/places/tests/bookmarks/" is the way testing
> it ?
> /****/

Yes, but you probably need to work out how to get the test working before you make any changes. If you continue having trouble there, you could consider joining the #introduction channel on our IRC servers, where people should be able to help - https://wiki.mozilla.org/IRC

Comment 15

[Riko Ho]

Ok, I work out how to get the test working first then, thanks for the clue

Comment 16

[Riko Ho]

./mach test toolkit/components/places/tests/bookmarks/
I got the answer myself,simple, it's running already
=====
............................
0:31.16 TEST_START: Thread-45 toolkit/components/places/tests/bookmarks/test_sync_fields.js
 0:31.17 TEST_START: Thread-44 toolkit/components/places/tests/bookmarks/test_savedsearches.js
 0:32.43 TEST_END: Thread-34 PASS
 0:32.73 TEST_END: Thread-37 PASS
 0:32.89 TEST_END: Thread-38 PASS
 0:33.83 TEST_END: Thread-39 PASS
 0:34.20 TEST_END: Thread-41 PASS
 0:34.28 TEST_END: Thread-31 PASS
 0:34.33 TEST_END: Thread-35 PASS
 0:35.28 TEST_END: Thread-43 PASS
 0:35.38 TEST_END: Thread-40 PASS
 0:35.40 TEST_END: Thread-42 PASS
 0:36.17 TEST_END: Thread-44 PASS
 0:37.48 TEST_END: Thread-45 PASS
 0:37.48 LOG: MainThread INFO INFO | Result summary:
 0:37.48 LOG: MainThread INFO INFO | Passed: 45
 0:37.48 LOG: MainThread INFO INFO | Failed: 0
 0:37.48 LOG: MainThread INFO INFO | Todo: 0
 0:37.48 LOG: MainThread INFO INFO | Retried: 0
 0:37.48 SUITE_END: MainThread 
Summary
=======

Ran 45 tests
Expected results: 45
Unexpected results: 0

OK
====
Go back to the bug ==> - in this bug you need to find the SQL statement mentioned above and replace it with PlacesUtils.history.makeGuid().

So, I need to find "GUID" with function "PlacesUtils.history.makeGuid()", not with "SELECT...." is that what you mean ?
Please make it clearer ?

Comment 17

[Mark Hammond [:markh] [:mhammond]]

(In reply to Riko Ho from comment #16)
> So, I need to find "GUID" with function "PlacesUtils.history.makeGuid()",
> not with "SELECT...." is that what you mean ?

Yes, that's correct.

Comment 18

[Riko Ho]

Ok, thanks again for the clue, I'll find out if it's possible.

Comment 19

[Ryan]

Attachment: Bug 1347452 - Assigned item.guid using PlacesUtils.history.makeGuid() in insertBookmark().

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

Comment 20

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8898174 [details]
Bug 1347452 - Assigned item.guid using PlacesUtils.history.makeGuid() in insertBookmark().

https://reviewboard.mozilla.org/r/169540/#review175114

This looks good to me, but it will need to be reviewed by Marco - he is off on PTO until next week, so it might take him a number of days for him to get to it. Please adjust the commit message as I mention below and re-upload the patch, and it should be ready to review.

::: commit-message-93238:1
(Diff revision 1)
> +BUG 1347452 - Assigned item.guid using PlacesUtils.history.makeGuid() in insertBookmark().

Please change this comment message to use "Bug" rather than "BUG", and append to the end " r?mak", which will flag Marco for review in the next version.

Comment 21

[Ryan]

Thanks Mark. I amended the commit message, however, I can't

`hg push review r?mak`

because Marco is away:

    error publishing review request 169538: Error publishing: Bugzilla error: Marco Bonardo [::mak] (Away 4-20 August) <mak77@...> is not currently accepting 'review' requests. (HTTP 500, API Error 225)

I will wait until the 20th to close out this bug.

Comment 22

[Mark Hammond [:markh] [:mhammond]]

(In reply to Ryan from comment #21)
> Thanks Mark. I amended the commit message, however, I can't
> 
> `hg push review r?mak`
> 
> because Marco is away:
> 
>     error publishing review request 169538: Error publishing: Bugzilla
> error: Marco Bonardo [::mak] (Away 4-20 August) <mak77@...> is not currently
> accepting 'review' requests. (HTTP 500, API Error 225)
> 
> I will wait until the 20th to close out this bug.

Ah, OK, seeing he is back soon, waiting a few days does seem like the best option - thanks!

Comment 23

[Mark Banner (:standard8)]

Ryan, Marco is back, so hopefully you'll be able to request review now :-)

Comment 24

[Ryan]

Comment on attachment 8898174 [details]
Bug 1347452 - Assigned item.guid using PlacesUtils.history.makeGuid() in insertBookmark().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169540/diff/1-2/

Comment 25

[Ryan]

Thanks Mark. Done.

Comment 26

[Marco Bonardo [:mak]]

Comment on attachment 8898174 [details]
Bug 1347452 - Assigned item.guid using PlacesUtils.history.makeGuid() in insertBookmark().

https://reviewboard.mozilla.org/r/169540/#review176198

LGTM, thank you!

Comment 27

[Ryan]

Thanks Marco. I don't have permission to push this to the test server or, at least, I don't think I have this permission yet. Can you push the commit along or tell me what I need to do?

Comment 28

[Marco Bonardo [:mak]]

Please mark the issue in mozreview as "fixed" and then we can land it through it.

Comment 29

[Ryan]

Thanks Marco. Sorry for missing that detail. You can use the try server now. If everything is good I'll add the checkin-needed keyword.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5c8e8679ef69
Assigned item.guid using PlacesUtils.history.makeGuid() in insertBookmark(). r=mak

Comment 31

[Marco Bonardo [:mak]]

I just landed the patch through autoland, thank you

Comment 32

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/5c8e8679ef69

Comment 33

[Mark Hammond [:markh] [:mhammond]]

Thanks Ryan, but this has already been checked in \o/