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

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

Attachment: 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

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Is it too late to kill HTMLIsIndexElement.form?

Comment 3

[Mounir Lamouri (:mounir)]

(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?

Comment 4

[Jesse Ruderman]

> 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.

Comment 5

[Johnny Stenback (:jst)]

Not a blocker.

Comment 6

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

Comment 7

[Mounir Lamouri (:mounir)]

With this patch, the assert will pass if the elemnet is a HTMLIsIndexElement.

Comment 8

[Olli Pettay [:smaug]]

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.

Comment 9

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

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?

Comment 10

[Mounir Lamouri (:mounir)]

(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?

Comment 11

[Mounir Lamouri (:mounir)]

Jonas, see comment 10.

Comment 12

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

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.

Comment 13

[Mounir Lamouri (:mounir)]

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).

Comment 14

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

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 15

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

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.

Comment 16

[Boris Zbarsky [:bzbarsky, bz on IRC]]

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.

Comment 17

[Mounir Lamouri (:mounir)]

Attachment: Tests

Let's add tests for that at least.

Comment 18

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

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)

Comment 19

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/90ee071c963e

With a=tests