Closed Bug 602117 Opened 15 years ago Closed 15 years ago

"ASSERTION: FindAncestorForm should not be called if @form is set!"

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b8
Tracking Status
blocking2.0 --- -

People

(Reporter: jruderman, Assigned: mounir)

References

Details

(Keywords: assertion, testcase)

Attachments

(3 files, 1 obsolete file)

Attached file testcase
###!!! ASSERTION: FindAncestorForm should not be called if @form is set!: '!HasAttr(kNameSpaceID_None, nsGkAtoms::form)', file content/html/content/src/nsGenericHTMLElement.cpp, line 976
Attached file stack trace
blocking2.0: --- → ?
Is it too late to kill HTMLIsIndexElement.form?
(In reply to comment #2) > Is it too late to kill HTMLIsIndexElement.form? That's in DOM Level 1 specifications. Seems hard to remove. However, this assert was expected but considering that isindex shouldn't be used so much, we (Jonas and I) were assuming that having this assert showing when @form is set was not a problem. Jesse, can you "ignore" some asserts with your fuzzer?
OS: Mac OS X → All
Hardware: x86 → All
> However, this assert was expected but considering that isindex shouldn't be > used so much, we (Jonas and I) were assuming that having this assert showing > when @form is set was not a problem. It's not the end of the world, but it is a bug: http://weblogs.mozillazine.org/roadmap/archives/2005/01/assertions_shou.html > Jesse, can you "ignore" some asserts with your fuzzer? Yes. My DOM fuzzer is currently ignoring 516 assertions associated with 346 bugs.
Not a blocker.
blocking2.0: ? → -
Attached patch Patch v1 (obsolete) — Splinter Review
Assignee: nobody → mounir.lamouri
Status: NEW → ASSIGNED
Attachment #489187 - Flags: review?(Olli.Pettay)
With this patch, the assert will pass if the elemnet is a HTMLIsIndexElement.
Comment on attachment 489187 [details] [diff] [review] Patch v1 I'm not sure why the assertion was added, and since it was added in bug 588683, perhaps Jonas could review this.
Attachment #489187 - Flags: review?(Olli.Pettay) → review?(jonas)
Comment on attachment 489187 [details] [diff] [review] Patch v1 A better patch would be to make createElement("isindex") create a plain nsHTMLElement when the HTML5 parser is enabled. Would you mind?
(In reply to comment #9) > Comment on attachment 489187 [details] [diff] [review] > Patch v1 > > A better patch would be to make createElement("isindex") create a plain > nsHTMLElement when the HTML5 parser is enabled. > > Would you mind? That would make isindex.form not working anymore? Isn't that too late for such a change?
Jonas, see comment 10.
Note that we're only talking about createElement("isindex") here. It's already the case that the parser doesn't create elements with .nodeName = isindex. So currently the only way to get at a "real" isindex element is using createElement. I'm willing to eat my hat if there's any websites out there that depend on that. So no, it's not too late to make such a change.
I've created bug 611352 so we don't create a HTMLIsIndexElement when createElement('isindex') is called. However, I would prefer to have this patch landed because there are some path that could still lead to this assert (with the old parser).
I don't think this is worth messing with. The html4 parser is disabled and even if it was reenabled the assertion is harmless.
Comment on attachment 489187 [details] [diff] [review] Patch v1 I don't think we should mess with this since it'll only fire from disabled code. And even if it gets re-enabled the assertion is harmless.
Attachment #489187 - Flags: review?(jonas)
Sicking, would you still eat your hat if the website happens to be somewhat newly created? If so, can I watch? ;) Past that, I think sicking's right. isindex is really rarely used to start with; no one is using it with modern DOM APIs, I would think.
Depends on: 611352
Attached patch TestsSplinter Review
Let's add tests for that at least.
Attachment #489187 - Attachment is obsolete: true
Attachment #490295 - Flags: review?(jonas)
Comment on attachment 490295 [details] [diff] [review] Tests I guess. Though I think we should remove it once we remove the isindex element. (Doesn't strictly need approval since it's NPOTB)
Attachment #490295 - Flags: review?(jonas)
Attachment #490295 - Flags: review+
Attachment #490295 - Flags: approval2.0+
Flags: in-testsuite+
Target Milestone: --- → mozilla2.0b8
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: