1343473 - WebExtensions bookmarks.getSubTree() omits parentId of requested node

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P3)

Description

[Jerry Krinock]

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________".

Comment 1

[Jerry Krinock]

Attachment: Proposed patch to fix this bug

Patched file passes eslint and gives expected results.

Comment 2

[Bob Silverberg [:bsilverberg]]

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

Comment 3

[Jerry Krinock]

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  :(

Comment 4

[Bob Silverberg [:bsilverberg]]

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.

Comment 5

[Jerry Krinock]

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?

Comment 6

[Bob Silverberg [:bsilverberg]]

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.

Comment 7

[Jerry Krinock]

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  :|

Comment 8

[Bob Silverberg [:bsilverberg]]

(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.

Comment 9

[Jerry Krinock]

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.

Comment 10

[Jerry Krinock]

Attachment: 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.

Comment 11

[Bob Silverberg [:bsilverberg]]

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?

Comment 12

[Jerry Krinock]

Attachment: Patch to fix this bug, without unnecessary 'if()'

The API specifies that parentId is "optional", so, yes, undefined is OK.

Comment 13

[Shane Caraveo (:mixedpuppy)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2123dcb9b1124bfef6824d2daf43c2d5fd0bf7ca

Comment 14

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8845961 [details] [diff] [review]
Patch to fix this bug, without unnecessary 'if()'

r+ provided my try push succeeds

Comment 15

[Shane Caraveo (:mixedpuppy)]

Oh, and please add the commit message as I did in the try push above.

Comment 16

[Jerry Krinock]

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.

Comment 17

[Shane Caraveo (: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!

Comment 18

[Jerry Krinock]

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

Comment 19

[Shane Caraveo (: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

Comment 20

[Jerry Krinock]

Attachment: 1343473-04-withMsg.diff

Comment 21

[Jerry Krinock]

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 :(

Comment 22

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8848280 [details] [diff] [review]
1343473-04-withMsg.diff

carry forward prior r+ from us

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0844d1f90bbc
Fixed parentId on the root node returned from bookmarks.getSubTree. r=bsilverberg, r=mixedpuppy

Comment 24

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

https://hg.mozilla.org/mozilla-central/rev/0844d1f90bbc