580814 - Error: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsINavHistoryContainerResultNode.getChild] I drag a link in the blank of the lower part of the Bookmark Sidebar, and drop it

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Alice0775 White]

User-Agent:       Mozilla/5.0 (Windows; Windows NT 6.1; WOW64; rv:2.0b3pre) Gecko/20100721 Minefield/4.0b3pre ID:20100721041129
Build Identifier: Mozilla/5.0 (Windows; Windows NT 6.1; WOW64; rv:2.0b3pre) Gecko/20100721 Minefield/4.0b3pre ID:20100721041129

When I drag a link in the blank of the lower part of the Bookmark Sidebar, and drop it

Error: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsINavHistoryContainerResultNode.getChild]
Source file: chrome://browser/content/places/treeView.js
Line: 250


Reproducible: Always

Steps to Reproduce:
1. Start Minefield with new profile
2. Open http://www.mozilla.org/projects/minefield/ 
3. Open Bookmarks Sidebar
4. Drag a link in the blank of the lower part of the sidebar, and drop it

Actual Results:
Error: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsINavHistoryContainerResultNode.getChild]
Source file: chrome://browser/content/places/treeView.js
Line: 250

Expected Results:
No error

Comment 1

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

I got the same error on a following environment.

Environment: Nightly build on http://hg.mozilla.org/mozilla-central/rev/a853f4017192.

STR:
1. Open error console & bookmark sidebar.
2. Close all tree.
3. Drag a item (e.g. tab, url, or bookmark item) to bookmark sidebar empty area.
   This empty area is red zone on uploaded image named "empty area image".

Result:
Error: uncaught exception: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsINavHistoryContainerResultNode.getChild]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: chrome://browser/content/places/treeView.js :: PTV__getNodeForRow :: line 260"  data: no]

I got to dump the stack trace of this error.
following it.

Error: PTV__getNodeForRow(-1)@chrome://browser/content/places/treeView.js:266
PTV__getParentByChildRow(-1)@chrome://browser/content/places/treeView.js:232
PTV_getParentIndex(-1)@chrome://browser/content/places/treeView.js:1295

Comment 2

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: empty area image

Comment 3

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: Proposed patch

Comment 4

[Marco Bonardo [:mak]]

(In reply to OHZEKI Tetsuharu [:saneyuki_s] from comment #1)

> Error: PTV__getNodeForRow(-1)@chrome://browser/content/places/treeView.js:266
> PTV__getParentByChildRow(-1)@chrome://browser/content/places/treeView.js:232
> PTV_getParentIndex(-1)@chrome://browser/content/places/treeView.js:1295

The problem with your patch is that getNodeForRow will return the root node, getParentByChildRow will try to get its parent, that is null and will pass null to other methods.

I think we should first figure out who is calling getParentIndex with -1 argument and fix the caller, since it makes no sense that it tries to get the parent for a nonexisting index.

Comment 5

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Marco Bonardo [:mak] from comment #4)
> The problem with your patch is that getNodeForRow will return the root node,
> getParentByChildRow will try to get its parent, that is null and will pass
> null to other methods.
> 
> I think we should first figure out who is calling getParentIndex with -1
> argument and fix the caller, since it makes no sense that it tries to get
> the parent for a nonexisting index.

At First, I thought same thing.

However, the current implementation of getNodeForRow does not allows that it called with -1 argument. This reported error happens too if someone calls getNodeForRow with -1 argument from other.

I think that we should fix getNodeForRow. To Fix getParentIndex does not fixing the root problem.

Comment 6

[Marco Bonardo [:mak]]

well, my assumption is that it should either throw or return null, the code is clearly asking for a row that does not exist, so it's not expected to work. Then, if we figure out it's a case we need, we may even provide a fallback, but I can't think of a case off-hand (that's where understanding why we pass -1 would be useful). Add-ons are likely already broken so a change here should not matter.

Comment 7

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Marco Bonardo [:mak] from comment #6)
> well, my assumption is that it should either throw or return null, the code
> is clearly asking for a row that does not exist, so it's not expected to
> work. Then, if we figure out it's a case we need, we may even provide a
> fallback, but I can't think of a case off-hand (that's where understanding
> why we pass -1 would be useful). Add-ons are likely already broken so a
> change here should not matter.

I think that changing getParentIndex or getParentByChildRow have same compatibility problem.

I assume that to fix getNodeForRow with my proposed patch is smallest change, and But this may causes broken compatibility.
Fixing getNodeForRow throw or return null will be safe. However we must fix getParentByChildRow or getParentIndex too.

Comment 8

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

nsITreeView.idl (http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/tree/public/nsITreeView.idl#119) describes "If there is no parent row, getParentIndex returns -1".

Comment 9

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

If we change getNodeForRow to return null,  What should getParentByChildRow returns as parent node when getNodeForRow returns null?
I think that getParentByChildRow should returns the root node on its case.

Comment 10

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: proposal 2

This proposal changes:
*PTV__getNodeForRow returns null when it's called with -1 argument.
*PTV__getParentByChildRow returns the root node as parent when it's called with -1 argument.

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 599192 [details] [diff] [review]
proposal 2

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

We still don't know who is passing -1 to getParentIndex, right?
The proposal 2 patch looks fine, though I would still like to know who is calling getParentIndex(-1), to fix it too in this patch. Any possibility to get a stack showing that? Does the stack go to cpp?

Comment 12

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Marco Bonardo [:mak] from comment #11)
> We still don't know who is passing -1 to getParentIndex, right?
> The proposal 2 patch looks fine, though I would still like to know who is
> calling getParentIndex(-1), to fix it too in this patch. Any possibility to
> get a stack showing that? Does the stack go to cpp?

Sorry, attaching patch 2 is too fast and sloppy.
This is in the assumption, I seem getParentIndex(-1) is called from cpp.
The reason I thought is two:
First, the stack ends PTV_getParentIndex(-1).
Second, PTV_getParentIndex(-1) may be called from nsTreeBodyFrame::HandleEvent (http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp#2732).
The error of this bug occurs when dropping an item to bookmark sidebar.
So nsTreeBodyFrame::HandleEvent is related to drag&drop event.
Third, PTV_getParentIndex is called from nsIPlacesView.removableSelectionRanges()(http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/tree.xml#401), however, I check with debugger, nsIPlacesView.removableSelectionRanges() does not call PTV_getParentIndex(-1).

Comment 13

[Marco Bonardo [:mak]]

Comment on attachment 599192 [details] [diff] [review]
proposal 2

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

ok, then yeah I think this change makes sense.

Comment 14

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Marco Bonardo [:mak] from comment #13)
> ok, then yeah I think this change makes sense.

Thanks, Marco.

Comment 15

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Why doesn't this patch check in?

Comment 16

[Josh Matthews [:jdm]]

You should add the checkin-needed keyword in the future so that reviewed patches are not forgotten. Please also remember to upload patches with r=[reviewer's name] in the commit message as well, as those are required for checking in. I changed the commit message to this:

"Bug 580814 - Avoid accessing rows that don't exist in treeviews for Places. r=mak"

since it describes the change being made much better than the title of the bug does. Nevertheless, thank you for the patch!

http://hg.mozilla.org/integration/mozilla-inbound/rev/058ac6b6930b

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/058ac6b6930b

Comment 18

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Josh Matthews [:jdm] from comment #16)
> You should add the checkin-needed keyword in the future so that reviewed
> patches are not forgotten. Please also remember to upload patches with
> r=[reviewer's name] in the commit message as well, as those are required for
> checking in. I changed the commit message to this:
> 
> "Bug 580814 - Avoid accessing rows that don't exist in treeviews for Places.
> r=mak"
> 
> since it describes the change being made much better than the title of the
> bug does. Nevertheless, thank you for the patch!
> 
> http://hg.mozilla.org/integration/mozilla-inbound/rev/058ac6b6930b

Thank you for your clarification, Josh.
I'm going to do so next time.