The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 13

Status

()

Firefox
Bookmarks & History
--
trivial
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: Alice0775 White, Assigned: tetsuharu)

Tracking

Trunk
Firefox 13
x86
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

7 years ago
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
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
Created attachment 598208 [details]
empty area image
Created attachment 598239 [details] [diff] [review]
Proposed patch
Attachment #598239 - Flags: review?(mak77)
Assignee: nobody → saneyuki.s.snyk
(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.
(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.
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.
(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.
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".
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.
Created attachment 599192 [details] [diff] [review]
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 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?
Attachment #599192 - Flags: feedback+

Updated

5 years ago
Attachment #598239 - Attachment is obsolete: true
Attachment #598239 - Flags: review?(mak77)
(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 on attachment 599192 [details] [diff] [review]
proposal 2

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

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

Thanks, Marco.
Why doesn't this patch check in?
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
https://hg.mozilla.org/mozilla-central/rev/058ac6b6930b
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 13
(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.
You need to log in before you can comment on or make changes to this bug.