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)
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•15 years ago
|
||
![]() |
||
Updated•15 years ago
|
blocking2.0: --- → ?
Comment 2•15 years ago
|
||
Is it too late to kill HTMLIsIndexElement.form?
Assignee | ||
Comment 3•15 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•15 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•15 years ago
|
||
Assignee: nobody → mounir.lamouri
Status: NEW → ASSIGNED
Attachment #489187 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 7•15 years ago
|
||
With this patch, the assert will pass if the elemnet is a HTMLIsIndexElement.
Comment 8•15 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•15 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•15 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•15 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•15 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•15 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•15 years ago
|
||
Pushed:
http://hg.mozilla.org/mozilla-central/rev/90ee071c963e
With a=tests
Flags: in-testsuite+
Target Milestone: --- → mozilla2.0b8
Assignee | ||
Updated•15 years ago
|
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.
Description
•