330879 - nodeIs* functions in Places controller give JS strict warning on non-node parameter

Status: RESOLVED INACTIVE

(Firefox :: Bookmarks & History, defect)

Description

[Joe Hughes]

When I accidentally passed in a non-node value (in this case the number 6) into nodeIsFolder(), I got a "Javascript strict warning: ... reference to undefined property node.type".  These functions should check whether there is a "type" property (perhaps whether the object is actually a result node) and return false if not.

Comment 1

[O. Atsushi (Torisugari)]

*** Bug 338795 has been marked as a duplicate of this bug. ***

Comment 2

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv

Comment 3

[:Paolo Amadini]

If the object passed in is not a Places node, this is an incorrect usage of the
API. Marco suggests we should just throw an informative exception in this case.

Comment 4

[Amod [:AMoz]]

Hi.. I would take this bug. Can you guide me on what steps should i undertake ?

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Hello, first of all, if you're not yet introduced to the process, please start by checking this page:
https://developer.mozilla.org/en-US/docs/Introduction

at which point are you, do you already have a working build?
You can also take a look at:
https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in

Regarding this bug, in PlacesUtils.jsm file you can find some nodeIs... methods, these don't verify if the passed in node is a Places node, you should ensure that and throw a better exception when needed.

Comment 6

[Amod [:AMoz]]

hi mak. I have a working build. Now I am heading towards Santa Clara for Mozilla Summit 2013. I will try to find time there. Else I would resume the work after returning.

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

well, I'm flying to SC tomorrow, as well. So, cya there :)

Comment 8

[Athira]

I am an absolute beginner.Can I work on this bug please?

Comment 9

[Amod [:AMoz]]

Athira.. I am extremely sorry since I have communicated with the mentor. I will let you know if I think of giving up the bug at later point of time.

Comment 10

[Amod [:AMoz]]

Hi Marco. I tried to search you in Mozilla Summit 2013 but in vain.. Anyways I would begin working on the bug within a day or two.

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Amod [:greatwarrior] from comment #10)
> Hi Marco. I tried to search you in Mozilla Summit 2013 but in vain.. Anyways
> I would begin working on the bug within a day or two.

I'm sorry, there were just too many persons!

Comment 12

[Amod [:AMoz]]

Hello mak. I had gone for a break and have returned now.
So I continue with this bug.
I observed that nodeIs methods are alled and not defined in the file.
And please give me more insight into 
>> these don't verify if the passed in node is a Places node

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Amod [:greatwarrior] from comment #12)
> Hello mak. I had gone for a break and have returned now.
> So I continue with this bug.
> I observed that nodeIs methods are alled and not defined in the file.
> And please give me more insight into 
> >> these don't verify if the passed in node is a Places node

Hi, welcome back.

You can find the methods that should be changed in this query:

http://mxr.mozilla.org/mozilla-central/search?string=nodeIS.*%3A&regexp=on&find=PlacesUtils&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

You see they take aNode as input, though there is no check that this is a valid Places node, rather than anything else (I could pass anything here, and I will get an error, as indicated in comment 0).
So, what we need here is a check at the beginning that will throw if something else is passed into.
I think that a simple if (aNode instanceof Ci.nsINavHistoryResultNode) should work.

Then it would be nice to have a simple xpcshell-test that calls them with invalid values and checks they throw as expected.

Comment 14

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

well it should be if (!(aNode instanceof....)) clearly :)

Comment 15

[Amod [:AMoz]]

Attachment: bug-330879-fix

I have made corresponding changes.

Comment 16

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8393538 [details] [diff] [review]
bug-330879-fix

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

As I said in previous comment, I'd really love a simple xpcshell-test that calls these methods and checks they throw if the wrong argument type is passed, don't throw otherwise.
This documentation should help you start with xpcshell-tests, https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests

you should add a test_PlacesUtils_nodeIsXXX_invalidArg.js to toolkit/components/places/tests/unit/ and to the xpcshell.ini file into that same folder
the test should have an array with names of all of the methods you are going to test
for each entry in the array you should invoke it with a wrong argument (like a string, a bool, or whatever else, doesn't matter)

::: toolkit/components/places/PlacesUtils.jsm
@@ +154,5 @@
>     * @param   aNode
>     *          A result node
>     * @returns true if the node is a Bookmark folder, false otherwise
>     */
> +  nodeIsFolder: function PU_nodeIsFolder(aNode) { 

you added a trailing space here, please remove it

@@ +155,5 @@
>     *          A result node
>     * @returns true if the node is a Bookmark folder, false otherwise
>     */
> +  nodeIsFolder: function PU_nodeIsFolder(aNode) { 
> +    if (!(aNode instanceof Ci.nsINavHistoryResultNode)){ return false; }

The condition looks correct, but per our coding style, you should indent if(s) as

if (condition) {
  statements...
}

Also, you should throw new Error("Invalid Places node"); instead of returning false.
This is because passing something that is not a Places node is a clear coding error, so we should try to not swallow it and make the consumer aware of the problem.
Clearly this comment should be applied to all of the changes.

Comment 17

[Amod [:AMoz]]

Attachment: bug-330879-fix

Sorry for the delay.
I have written a test as well but that fails a lot. I am not getting suitable reason to it.

Comment 18

[Amod [:AMoz]]

Attachment: test_PlacesUtils_nodeIsXXX_invalidArg.js

the code i wrote gives a failure.
It would be great if I get some guidance. I am trying xpcshell test for the first time.

Comment 19

[Amod [:AMoz]]

Attachment: bug-330879-fix

Sorry, by mistake I remarked the new patch as obsolete..So uploading again.

Comment 20

[:Gavin Sharp [email: [e-mail]]]

Looks like the latest patch doesn't include test_PlacesUtils_nodeIsXXX_invalidArg.js itself - you probably forgot to "hg add" it?

Comment 21

[Amod [:AMoz]]

Attachment: bug-330879-fix

I have added the tests to the patch.

Comment 22

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8405724 [details] [diff] [review]
bug-330879-fix

you don't need 2 reviewers for this, Gavin just happened to notice an issue in the patch, that happens pretty often, but you don't need to ask him for further checks.

Comment 23

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8405724 [details] [diff] [review]
bug-330879-fix

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

::: toolkit/components/places/PlacesUtils.jsm
@@ +155,5 @@
>     *          A result node
>     * @returns true if the node is a Bookmark folder, false otherwise
>     */
>    nodeIsFolder: function PU_nodeIsFolder(aNode) {
> +    if (!(aNode instanceof Ci.nsINavHistoryResultNode)){

per our coding style, you should add a whitespace before the {

(for all of the instances in the patch)

::: toolkit/components/places/tests/unit/test_PlacesUtils_nodeIsXXX_invalidArg.js
@@ +1,1 @@
> +

you should add a license header to the test file, either a public domain header like

/* Any copyright is dedicated to the Public Domain.
   http://creativecommons.org/publicdomain/zero/1.0/ */

or an MPL2 header like

/* This Source Code Form is subject to the terms of the Mozilla Public
 * License, v. 2.0. If a copy of the MPL was not distributed with this
 * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

@@ +1,2 @@
> +
> +Components.utils.import("resource://places/PlacesUtils.jsm");

you don't need to do this, cause any test in toolkit/components/places/tests/unit includes head_bookmarks.js from the same folder, and that includes toolkit/components/places/tests/head_common.js... basically any Places xpcshell-test includes head_common.js that already imports PlacesUtils (and many other modules or helpers)
This is a special setup you may not commonly find in other modules, though in any module xpcshell-tests include head files (as defined in xpcshell.ini)

@@ +1,5 @@
> +
> +Components.utils.import("resource://places/PlacesUtils.jsm");
> +
> +function run_test() {
> + var nodeIsMethods =[PU_nodeIsFolder(true)];

please use "let" instead of "var" in new code
also missing whitespace after the "=" per coding style

here you probably wanted to provide a list of names of the methods to be verified in PlacesUtils like

let nodeIsMethods = [
  "nodeIsFolder",
  "nodeIsBookmark",
  (and so on...)
];

@@ +8,5 @@
> +  nodeIsMethods[i];
> + }
> + }catch(ex) {
> + console.log(ex);
> + };

and here you want to call the method and check it fails:

for (let methodName of nodeIsMethods) {
  try {
    PlacesUtils[methodName](true);
    do_throw("Should have thrown");
  }
  catch (ex) {
    // We are expecting this to fail.
  }
}

note that for...of... construct is safer for arrays, since it won't list properties like for...in..., so it's more resistant to modified Array prototypes

::: toolkit/components/places/tests/unit/xpcshell.ini
@@ +139,5 @@
>  [test_getPlacesInfo.js]
>  [test_pageGuid_bookmarkGuid.js]
>  [test_async_transactions.js]
> +
> +[test_PlacesUtils_nodeIsXXX_invalidArg.js]

please remove the additional newline before this

Comment 24

[Amod [:AMoz]]

>>do_throw("Should have thrown");
I am not getting the significance of this stmnt. Why we should manually throw the exception ? It should be thrown when the method's parameter is invalid.

The test PASSES when i remove the above stmnt.

Comment 25

[David Teller [:Yoric] - still alive but not very active]

(In reply to Amod [:greatwarrior] from comment #24)
> >>do_throw("Should have thrown");
> I am not getting the significance of this stmnt. Why we should manually
> throw the exception ? It should be thrown when the method's parameter is
> invalid.
> 
> The test PASSES when i remove the above stmnt.

do_throw() causes the test to fail if you reach that line, i.e. if the previous line hasn't thrown.

Comment 26

[:Paolo Amadini]

I'll add that we have recently added a new way to check that a given call throws an exception, the "Assert.throws" method:

https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests#Assertions_and_logging

Using an arrow function, the same code example becomes:

for (let methodName of nodeIsMethods) {
  Assert.throws(() => PlacesUtils[methodName](true),
                /Invalid Places node/);
}

Comment 27

[Amod [:AMoz]]

Attachment: patchv2

Thank you Yoric for the help. 
Thank you Paolo for suggesting the "Assert.throws" method. I have followed the syntax given in the link and wrote the entire method call on the same line along with the exception.

Comment 28

[Amod [:AMoz]]

Attachment: patchv3

Removed an extra whitespace.

Comment 29

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8407517 [details] [diff] [review]
patchv3

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

looks good, thank you, and everyone who helped mentoring this.

if you don't have commit privilege to the tree, feel free to set the checkin-needed keyword so that our sheriffs will take care of landing your patch.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/566c01fe0d98

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/566c01fe0d98

Comment 32

[Amod [:AMoz]]

thank you very much. Looking forward to work with you again :)

Comment 33

[Alice0775 White]

This should be backed out, because livebookmark completely broken.
I wonder, there is not any automate test.

Comment 34

[Amod [:AMoz]]

Alice..Lets consult Marco regarding this.
Should I contribute to bug 997976 ?

Comment 35

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

hm, yeah, my fault!
live bookmarks "simulate" a places node, but the node it not an instanceof nsINavHistoryResultNode.

I will backout this and we should work on an alternative solution. sorry!

Comment 36

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

backed out per the regression in bug 997976
https://hg.mozilla.org/integration/fx-team/rev/75f108774d93

now, I think we should find a less precise but still meaningful way to distinguish the node. We may try with
if (!QI_node(aNode, Ci.nsINavHistoryResultNode)) {
  throw...
}
I think this should work for both kind of nodes (also the fake nodes provide a QueryInterface method), though please verify that:
1. it works (the new unit test passes)
2. live bookmarks still work properly, try to add a new feed, open it, wait for it to load, right click on an entry and open in a new tab.

Please also file a follow-up bug to write a test for basic UI functionality of live bookmarks. Off-hand that may be tricky and take a little bit of effort to write a stable test.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/75f108774d93

Comment 38

[Asaf Romano (gone)]

Marco, I think we should create a base "faked node" prototype in PlacesUtils and do instanceof checks for it if the xpconnect-type instanceof doesn't work.

Comment 39

[Asaf Romano (gone)]

On a second thought, I filed bug 1016202 for wrapping livemark nodes with xpconnect.

Comment 40

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

unmentoring and unassigning since it's not ready to be worked on in the current status. Need to evaluate approaches (see comment 38/39)

Comment 41

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.