Closed
Bug 602117
Opened 14 years ago
Closed 14 years ago
"ASSERTION: FindAncestorForm should not be called if @form is set!"
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
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)
228 bytes,
text/html
|
Details | |
870 bytes,
text/plain
|
Details | |
954 bytes,
patch
|
sicking
:
review+
sicking
:
approval2.0+
|
Details | Diff | Splinter Review |
###!!! ASSERTION: FindAncestorForm should not be called if @form is set!: '!HasAttr(kNameSpaceID_None, nsGkAtoms::form)', file content/html/content/src/nsGenericHTMLElement.cpp, line 976
Reporter | ||
Comment 1•14 years ago
|
||
Updated•14 years ago
|
blocking2.0: --- → ?
Comment 2•14 years ago
|
||
Is it too late to kill HTMLIsIndexElement.form?
Assignee | ||
Comment 3•14 years ago
|
||
(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
Reporter | ||
Comment 4•14 years ago
|
||
> 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.
Assignee | ||
Comment 6•14 years ago
|
||
Assignee: nobody → mounir.lamouri
Status: NEW → ASSIGNED
Attachment #489187 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 7•14 years ago
|
||
With this patch, the assert will pass if the elemnet is a HTMLIsIndexElement.
Comment 8•14 years ago
|
||
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?
Assignee | ||
Comment 10•14 years ago
|
||
(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?
Assignee | ||
Comment 11•14 years ago
|
||
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.
Assignee | ||
Comment 13•14 years ago
|
||
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)
Comment 16•14 years ago
|
||
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.
Assignee | ||
Comment 17•14 years ago
|
||
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+
Assignee | ||
Comment 19•14 years ago
|
||
Pushed: http://hg.mozilla.org/mozilla-central/rev/90ee071c963e With a=tests
Flags: in-testsuite+
Target Milestone: --- → mozilla2.0b8
Assignee | ||
Updated•14 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•