Closed Bug 602117 Opened 9 years ago Closed 9 years ago

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

Categories

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

defect
Not set

Tracking

()

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

People

(Reporter: jruderman, Assigned: mounir)

References

(Blocks 2 open bugs)

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+
Pushed:
http://hg.mozilla.org/mozilla-central/rev/90ee071c963e

With a=tests
Flags: in-testsuite+
Target Milestone: --- → mozilla2.0b8
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.