The default bug view has changed. See this FAQ.

WebExtensions bookmarks.getSubTree() omits parentId of requested node

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
WebExtensions: General
P3
normal
RESOLVED FIXED
28 days ago
11 days ago

People

(Reporter: Jerry Krinock, Assigned: bsilverberg)

Tracking

54 Branch
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [bookmarks] triaged)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

28 days ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_4) AppleWebKit/603.1.30 (KHTML, like Gecko) Version/10.1 Safari/603.1.30

Steps to reproduce:

• Craft a WebExtension which, upon loading, invokes browser.bookmarks.getSubTree(), passing in as the parameter the guid of any folder which is not the root.  For example, pass in "toolbar_____".
• Run Firefox 54 Nightly.
• Load the extension.
• Look at the console and see what was retrieved by getSubTree().




Actual results:

The root node in the result (in our example, the Bookmarks Toolbar) does not have a `parentId` property.


Expected results:

All nodes except the root have a parent, so there should have be a `parentId` property.  In our example, the parentId property of the Bookmarks Toolbar should be returned as "root________".
(Reporter)

Comment 1

28 days ago
Created attachment 8842327 [details] [diff] [review]
Proposed patch to fix this bug

Patched file passes eslint and gives expected results.

Updated

23 days ago
Assignee: nobody → bob.silverberg
Whiteboard: [bookmarks], triaged
Thanks for the bug report and the patch, Jerry. I verified that this bug does in fact exist, and your patch fixes it, but I think the ideal solution is a bit different. 

The `convert` function inside `getTree` already includes logic to add the `parentId` to the current node, so we can just use that, but I think it's currently flawed. So instead of calling `convert` and then adding the `parentId`, as you do in your patch, we should just fix `convert`. 

From what I can tell, `convert` should not accept the `parent` argument at all, as it's not needed. The node passed in will have a `parentGuid` which is the only information we need about the parent. The test at line 30 [1] could be updated to simply check:

`if (node.guid != PlacesUtils.bookmarks.rootGuid)` 

and then if that's true we can just set 

`treenode.parentId = node.parentGuid;`

Do you think you can update your patch to do that?  

Also, in order to land this we should have a test which proves that it works. That test should fail when run without the patch, and then pass with the patch applied. Do you think you could write such a test? It would be added to the existing tests in test_ext_bookmarks.js. [2]

[1] http://searchfox.org/mozilla-central/source/browser/components/extensions/ext-bookmarks.js#30
[2] http://searchfox.org/mozilla-central/source/browser/components/extensions/test/xpcshell/test_ext_bookmarks.js
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(jerry)
Priority: -- → P3
Whiteboard: [bookmarks], triaged → [bookmarks] triaged
(Reporter)

Comment 3

22 days ago
Hello, Bob.

Your suggestion is really clean and I wish it would work.  Unfortunately, due to the wonkiness of PlacesUtils, it breaks more than it fixes.  You see that `node` object is a resolved value from PlacesUtils.promiseBookmarksTree(), which we can read about in here:

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

Starting at line 1733, documentation lists the properties we can expect.  You see parentGuid is not among them.  Starting at line 1749 it lists two additional properties, parentGuid among them, which "the root object (i.e. the one for aItemGuid) **also** has … set"  ==>  All objects *except* the root object do *not* have parentGuid set  :(

And, indeed, it behaves exactly as documented.  So, although your suggestion fixes this bug of no parentId in the target root, it breaks parentId for every other object in the tree.

Regarding those tests, yes I should like to work on those and in fact need to do so for other bugs.  Just tell me how to run them please.  After looking at some documentation, I guessed:

`mach mochitest source/browser/components/extensions/test/xpcshell/test_ext_bookmarks.js`

in mozilla-central.  But after grinding for 189 seconds it said "could not find any mochitests" in there  :(
Flags: needinfo?(jerry) → needinfo?(bob.silverberg)
Ah, I totally missed that, thanks Jerry! In that case yes, your solution seems sound. As for the tests, they are not mochitests, they are xpcshell tests, so you either need to use `mach xpcshell-test` or just simply `mach test` to run them.
Flags: needinfo?(bob.silverberg)
(Reporter)

Comment 5

22 days ago
Thank you, Bob.  I think I just need a pointer or two to get started on this testing stuff.

Several weeks ago I cloned mozilla-central, and I did a pull on some random day a couple weeks ago.  It builds and runs fine.  `hg diff` returns nothing.  I just did another `mach build` and it completed successfully.  The 'Nightly' product seems to work OK.  Now when I run this command:

   mach xpcshell-test source/browser/components/extensions/test/xpcshell/test_ext_bookmarks.js

after a few minutes, I get several lines indicating that no tests were run.  Similar result if I do `mach test …`.   See transcripts here: http://pastebin.com/YxExgwh8 (for 1 month).

When I instead go for the whole enchilada:

   mach xpcshell-test

I get lots of output as expected, 2.3 MB, but it includes 4500 occurrences of the word "FAIL".  I am expecting zero FAILs.

What am I doing wrong?
Flags: needinfo?(bob.silverberg)
Does the file test_ext_bookmarks.js definitely exist at source/browser/components/extensions/test/xpcshell/test_ext_bookmarks.js? The output you pasted looks like the message one would get if one passes an invalid test location to mach xpcshell-test.
Flags: needinfo?(bob.silverberg)
(Reporter)

Comment 7

21 days ago
Ah, thank you.  That first path component, 'source/' shouldn't have been there.

* * *

With that fix, I'm getting slightly better results.  Instead of "0 tests", I'm getting 0 or 1 PASS, and 1 FAIL - see http://pastebin.com/Pn8eRHrm (for 1 month).  But the one failure in there:

JavaScript error: /Volumes/TM3-2/Projects/Firefox/mozilla-central/testing/xpcshell/head.js, line 30: NS_ERROR_FILE_NOT_FOUND: Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import]

does not look anything like any of the 144 browser.test.assertXxxx calls I see in test_ext_bookmarks.js.  What can we make of that?

Oh, I did another `hg pull` and `mach build`, thinking maybe I'd pulled things on a bad day last time.  No change.

Also, which of the following, A or B, is the best incantation for this purpose?

A)  mach          test browser/components/extensions/test/xpcshell/test_ext_bookmarks.js
B)  mach xpcshell-test browser/components/extensions/test/xpcshell/test_ext_bookmarks.js

Thank you for your patience.  It seems like running tests should not be too hard, but I'm not finding much helpful documentation  :|
Flags: needinfo?(bob.silverberg)
(In reply to Jerry Krinock from comment #7)
> Ah, thank you.  That first path component, 'source/' shouldn't have been
> there.
> 
> * * *
> 
> With that fix, I'm getting slightly better results.  Instead of "0 tests",
> I'm getting 0 or 1 PASS, and 1 FAIL - see http://pastebin.com/Pn8eRHrm (for
> 1 month).  But the one failure in there:
> 
> JavaScript error:
> /Volumes/TM3-2/Projects/Firefox/mozilla-central/testing/xpcshell/head.js,
> line 30: NS_ERROR_FILE_NOT_FOUND: Component returned failure code:
> 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import]
> 

I'm not sure what is causing that, but I am wondering if you've done a:

mach clobber

followed by a:

mach build

after pulling the latest from mozilla-central?

It's pretty important to do that, so if you haven't please try that and see if it fixes the error you are seeing.

> 
> Also, which of the following, A or B, is the best incantation for this
> purpose?
> 
> A)  mach          test
> browser/components/extensions/test/xpcshell/test_ext_bookmarks.js
> B)  mach xpcshell-test
> browser/components/extensions/test/xpcshell/test_ext_bookmarks.js
>

Those will both run the test. The only difference is that the output from A may be different from B, but either is fine to run the actual test.

> Thank you for your patience.  It seems like running tests should not be too
> hard, but I'm not finding much helpful documentation  :|

No problem, thanks for sticking with it.
Flags: needinfo?(bob.silverberg)
(Reporter)

Comment 9

20 days ago
Thank you, Bob.

Tests are working now.  The clobber, pull and (re)-build did not help the first time, but after I removed an old .mozconfig file, then did the clobber and build again, then test again, got good results…

  Ran 322 tests (1 parents, 321 subtests)
  Expected results: 322
  Unexpected results: 0

My old .mozconfig seems to have directed that xulrunner be built and tested.  Some recently-updated documentation I just read says that .mozconfig files are now discouraged  :)

Probably I'll be back in a day or so with a patch on that test_ext_bookmarks.js.
(Reporter)

Comment 10

19 days ago
Created attachment 8845713 [details] [diff] [review]
Patch to fix this bug, including patch for unit testing this time

Bob, I didn't set any "flags" on the patch, because I'm not sure of the correct procedure.  Please advance this to the next step.
Attachment #8842327 - Attachment is obsolete: true
Flags: needinfo?(bob.silverberg)
Comment on attachment 8845713 [details] [diff] [review]
Patch to fix this bug, including patch for unit testing this time

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

This looks good Jerry, nice work. :)

I added one comment that I'd like you to address but then I think it's good to go. I cannot give it final approval to land, and also I will be away for the next 2 weeks, so once you've addressed my comment, can you please flag it for review from one of my team members who is able to give it a final review? I suggest mixedpuppy.

::: browser/components/extensions/ext-bookmarks.js
@@ +58,5 @@
>        let children = root.children || [];
>        return children.map(child => convert(child, root));
>      }
> +    let treenode = convert(root, null);
> +    let rootParentGuid = root.parentGuid;

This seems unnecessary. Can you just change the lines below to:

if (root.parentGuid) {
    treenode.parentId = root.parentGuid;
}

Or, could you even just omit the `if` entirely? Would it be valid to have `treenode.parentId` be undefined when root.parentGuid also doesn't exist?
Attachment #8845713 - Flags: feedback+
Flags: needinfo?(bob.silverberg)
(Reporter)

Comment 12

19 days ago
Created attachment 8845961 [details] [diff] [review]
Patch to fix this bug, without unnecessary 'if()'

The API specifies that parentId is "optional", so, yes, undefined is OK.
Attachment #8845713 - Attachment is obsolete: true
Attachment #8845961 - Flags: review?(mixedpuppy)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2123dcb9b1124bfef6824d2daf43c2d5fd0bf7ca
Comment on attachment 8845961 [details] [diff] [review]
Patch to fix this bug, without unnecessary 'if()'

r+ provided my try push succeeds
Attachment #8845961 - Flags: review?(mixedpuppy) → review+
Oh, and please add the commit message as I did in the try push above.
(Reporter)

Comment 16

15 days ago
Shane, if Comment 15 was meant for me, I'm sorry I don't know to do that – it looks to me like you've already executed the commit.
Flags: needinfo?(mixedpuppy)
(In reply to Jerry Krinock from comment #16)
> Shane, if Comment 15 was meant for me, I'm sorry I don't know to do that –

no problem, it's just something needed before it can land.

> it looks to me like you've already executed the commit.

no, I just pushed it to the try server so the tests can run.  Looks like the tests passed, so you can add the commit message into the patch and attach it, then add checkin-needed to the keywords.

And than you for the fix!
Flags: needinfo?(mixedpuppy)
(Reporter)

Comment 18

13 days ago
One more dumb question, Shane.  How do you "add the commit message into the patch"?  Can I just paste in text at the top of a .diff file?

And as long as I've already stolen your attention, would the following be an acceptable Commit Message?

Bug 1343473 - In WebExtensions API browser.bookmarks.getSubTree(), added missing assignment to the parentId property of the root node of the returned subtree.  Also added a unit test for this. r?mixedpuppy
Flags: needinfo?(mixedpuppy)
Yeah, I think should be able to add the text followed by a single blank line before the diff.


Bug 1343473 fixed parentId on the root node returned from bookmarks.getSubTree.  r=bsilverberg,mixedpuppy
# blank line
# diff start
Flags: needinfo?(mixedpuppy)
(Reporter)

Comment 20

12 days ago
Created attachment 8848280 [details] [diff] [review]
1343473-04-withMsg.diff
Attachment #8848280 - Flags: review?(mixedpuppy)
Attachment #8848280 - Flags: review?(bob.silverberg)
(Reporter)

Comment 21

12 days ago
Diff with commit message at top is attached.

Shane, regarding your request to "add checkin-needed to the keywords":  Bugzilla documentation I've found implies that bug "keywords" are added in the "Tracking" section, and that not all people have access to it.  It appears that I do not have such access :(
Flags: needinfo?(mixedpuppy)
Comment on attachment 8848280 [details] [diff] [review]
1343473-04-withMsg.diff

carry forward prior r+ from us
Flags: needinfo?(mixedpuppy)
Attachment #8848280 - Flags: review?(mixedpuppy)
Attachment #8848280 - Flags: review?(bob.silverberg)
Attachment #8848280 - Flags: review+
Keywords: checkin-needed
Attachment #8845961 - Attachment is obsolete: true

Comment 23

11 days ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0844d1f90bbc
Fixed parentId on the root node returned from bookmarks.getSubTree. r=bsilverberg, r=mixedpuppy
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0844d1f90bbc
Status: NEW → RESOLVED
Last Resolved: 11 days ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.