374334 - Uncaught exception with cloneNode method and frameset element

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, I'm getting this uncaught exception in current trunk builds:
Error: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMHTMLFrameSetElement.cloneNode]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)" 

I should get an alert with "[object HTMLFrameSetElement]", which I get with current branch builds.

This regressed between 2005-10-27 and 2005-10-28:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-10-27+04&maxdate=2005-10-28+06&cvsroot=%2Fcvsroot
Not sure which which bug might have caused this: bug 311827 or bug 308270 or bug 264308.

Comment 1

[Alex Vincent [:WeirdAl]]

Attachment: stack trace where the failure unwinds from

So, because there's an error in the script for the onclick attribute, everything unwinds until it ultimately causes the cloneNode operation to fail.

The error is not reported the second time around (very interesting).

Comment 2

[Alex Vincent [:WeirdAl]]

Attachment: comparison stack (where the error was thrown from)

The top six lines of this stack are identical.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So someone on the callstack should not be propagating the error here. Probably nsEventListenerManager::AddScriptEventListener or maybe nsGenericHTMLElement::AfterSetAttr. Additionally it would be good if nsJSContext::CompileEventHandler threw a better error so that whoever is stopping it could do so easier.

Alex, do you want to have a go at this one?

Comment 4

[Alex Vincent [:WeirdAl]]

Yeah, I'll take a look at it sometime over the next 3-7 days.

Comment 5

[Alex Vincent [:WeirdAl]]

Attachment: patch (clarification on error needed)

This patch does fix the bug, but I don't know if using a DOM error code is appropriate for nsJSEnvironment.cpp.  jst, what do you think we should return here?  If we need a new error code, which .h file should define it and what should we call it?

Comment 6

[Johnny Stenback (:jst)]

Hmm, good questions. How about just using NS_ERROR_INVALID_VALUE in this case? Not ideal, but it's an existing error code that we could just use with enough of a similar meaning that we could pull that off. I'd think at least.

Comment 7

[Alex Vincent [:WeirdAl]]

jst: NS_ERROR_INVALID_VALUE doesn't exist according to lxr.  Did you mean NS_ERROR_ILLEGAL_VALUE?

Comment 8

[Johnny Stenback (:jst)]

Yeah, duh. I mixed up NS_ERROR_INVALID_ARG and NS_ERROR_ILLEGAL_VALUE.

Comment 9

[Alex Vincent [:WeirdAl]]

Attachment: updated patch

per jst's comments.

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 262068 [details] [diff] [review]
updated patch

Jonas, if you have any problems with this patch, please speak up now :)

r+sr=jst

Comment 11

[Alex Vincent [:WeirdAl]]

We ought to mochikit this test.

Comment 12

[Shawn Wilsher :sdwilsh]

Checking in dom/src/base/nsJSEnvironment.cpp;
new revision: 1.327; previous revision: 1.326
Checking in content/events/src/nsEventListenerManager.cpp;
new revision: 1.276; previous revision: 1.275