uncaught exception when trying to use nsISelectionController.selectAll

REOPENED
Unassigned

Status

()

Core
Selection
REOPENED
10 years ago
a year ago

People

(Reporter: Martijn Wargers (zombie), Unassigned)

Tracking

({testcase})

Trunk
x86
Windows XP
testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

10 years ago
Created attachment 338092 [details]
testcase (uses enhanced privileges)

See testcase, I get this error when trying to use selectAll() method from nsISelectionController:
Error: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsISelectionController.selectAll]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: file:///C:/Documents%20and%20Settings/mw/Bureaublad/caret_draw_selectioncontroller.htm :: doe :: line 20"  data: no]

I don't think that should happen
(Reporter)

Comment 1

10 years ago
Created attachment 338093 [details] [diff] [review]
patch

Removing this code:
-  //HACKHACKHACK
-  if (!aNewFocus->GetParent())
-    return NS_ERROR_FAILURE;
-  //END HACKHACKHACK /checking for root frames/content
-
 in nsFrameSelection::TakeFocus seems to make this work. I have no idea if it's safe to remove that, though.
Attachment #338093 - Flags: review?(uriber)
I tracked this code back to where it was submitted, which was on January 1999, with the comment "selection should work now more or less":

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/layout/base/src/Attic&command=DIFF&root=/cvsroot&file=nsRangeList.cpp&rev1=1.14&rev2=1.15#7

So not too much help from there :-)

I'll have a closer look at this later.

Updated

10 years ago
Attachment #338093 - Flags: review?(uriber) → review+
Comment on attachment 338093 [details] [diff] [review]
patch

I'm generally fine with this, assuming that we really want SelectAll() to select the entire document (including the <head> element!). Note that invoking "Select All" through the UI doesn't trigger this code, but rather just selects all children of the <body> tag.

Also, I found at least one piece of code that doesn't handle an entire-document selection well: notice that trying to copy the selection created by SelectAll() (with your fix) doesn't work. The failure is in the following stack trace:

#0  nsHTMLCopyEncoder::GetNodeLocation (this=0x1d4ade90, inChild=0x171aad3c, outParent=0xbfffce70, outOffset=0xbfffce6c) at /Users/uri/mozilla/central/mozilla/content/base/src/nsDocumentEncoder.cpp:1663
#1  0x11691963 in nsHTMLCopyEncoder::GetPromotedPoint (this=0x1d4ade90, aWhere=nsHTMLCopyEncoder::kStart, aNode=0x171aad3c, aOffset=0, outNode=0xbfffcf30, outOffset=0xbfffcf24, common=0xcdca8c) at /Users/uri/mozilla/central/mozilla/content/base/src/nsDocumentEncoder.cpp:1504
#2  0x11692249 in nsHTMLCopyEncoder::PromoteAncestorChain (this=0x1d4ade90, ioNode=0xbfffcfb0, ioStartOffset=0xbfffcfa8, ioEndOffset=0xbfffcfa4) at /Users/uri/mozilla/central/mozilla/content/base/src/nsDocumentEncoder.cpp:1412
#3  0x11692aaa in nsHTMLCopyEncoder::PromoteRange (this=0x1d4ade90, inRange=0x1717324c) at /Users/uri/mozilla/central/mozilla/content/base/src/nsDocumentEncoder.cpp:1373
#4  0x11695d0c in nsHTMLCopyEncoder::SetSelection (this=0x1d4ade90, aSelection=0x1712aa00) at /Users/uri/mozilla/central/mozilla/content/base/src/nsDocumentEncoder.cpp:1228
#5  0x1165cedb in nsCopySupport::HTMLCopy (aSel=0x1712aa00, aDoc=0xcdca00, aClipboardID=1) at /Users/uri/mozilla/central/mozilla/content/base/src/nsCopySupport.cpp:117
#6  0x11403aa4 in PresShell::DoCopy (this=0xb09c00) at /Users/uri/mozilla/central/mozilla/layout/base/nsPresShell.cpp:4242
(More stack frames follow...)

but the root cause is probably that nsHTMLCopyEncoder::IsRoot() looks for a "body" element, but not an "html" element (or a document).

Please submit a follow-up bug on this if you check in this patch.
(Reporter)

Comment 4

10 years ago
(In reply to comment #3)
> (From update of attachment 338093 [details] [diff] [review])
> I'm generally fine with this, assuming that we really want SelectAll() to
> select the entire document (including the <head> element!). Note that invoking
> "Select All" through the UI doesn't trigger this code, but rather just selects
> all children of the <body> tag.

Yeah, that's what I was thinking too. Maybe the SelectAll() function needs to do that instead? Nobody noticed that it was broken anyway.
(Reporter)

Comment 5

10 years ago
It seems to me, the error you described in comment 3, could also happen with some specially crafted script that tinkers with selection.
I tried to come up with some code that would trigger the same error as you mentioned in comment 3, but I get a different error, which I filed as bug 454904. Or is it the same error you described in comment 3?
(In reply to comment #4)

> Yeah, that's what I was thinking too. Maybe the SelectAll() function needs to
> do that instead? Nobody noticed that it was broken anyway.

Sounds like a good idea, although seems non-trivial to implement... currently "select all" goes through to different paths depending on whether you're doing it on the entire do or inside a text control (both of these end up in SelectAllChildren), so I think we'll have to replicate the logic for both of them here...

Sorry, looking at this mess of code really depresses me. I might take another shot at it when I'm in a better mood.
(In reply to comment #5)
> It seems to me, the error you described in comment 3, could also happen with
> some specially crafted script that tinkers with selection.
> I tried to come up with some code that would trigger the same error as you
> mentioned in comment 3, but I get a different error, which I filed as bug
> 454904. Or is it the same error you described in comment 3?

I'm pretty sure it's not the same error. I'll have a look at that one too.
(Reporter)

Updated

a year ago
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → WORKSFORME
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
You need to log in before you can comment on or make changes to this bug.