590387 - Crash [@ nsGenericHTMLFormElement::UpdateFormOwner] when attaching an element with @form set to an element which is not in a document

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

###!!! ASSERTION: When adding a form id observer, we should be in a document!: 'GetCurrentDoc()', file content/html/content/src/nsGenericHTMLElement.cpp, line 2815

###!!! ASSERTION: The element should be in a document when UpdateFormOwner is called!: 'GetCurrentDoc()', file content/html/content/src/nsGenericHTMLElement.cpp, line 2901

Null deref [@ nsGenericHTMLFormElement::UpdateFormOwner]

Comment 1

[Jesse Ruderman]

I think web pages and bookmarklets are somewhat likely to hit this bug.

Comment 2

[Jesse Ruderman]

Attachment: stacks

Comment 3

[Boris Zbarsky [:bzbarsky]]

The assumption that aBindToTree implies non-null current doc in UpdateFormOwner seems bogus to me.

Comment 4

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

Marking blocking beta6, but it'd be nice to fix this asap.

Comment 5

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

Comment 6

[Boris Zbarsky [:bzbarsky]]

I can't quite figure out what the comments mean to say....

Comment 7

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.1

Trying to improving the comment.

Comment 8

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

Comment on attachment 468981 [details] [diff] [review]
Patch v1

>+  // We @form is set, we *have* to be in a document otherwise, we will have no

When @form is set, we have to be in the document, otherwise we will have no form owner.

>+  // form owner.
>+  // If @form isn't set, we *need* a parent otherwise we are sure to not found
>+  // any form owner.
>+  if ((HasAttr(kNameSpaceID_None, nsGkAtoms::form) && GetCurrentDoc()) ||
>+      (!HasAttr(kNameSpaceID_None, nsGkAtoms::form) && aParent)) {
>     UpdateFormOwner(true, nsnull);
>   }

Don't call HasAttr twice. One solution could be:

if (HasAttr(...) ? !!GetCurrentDoc() : !!aParent)

Though really, adding a nullcheck to UpdateFormOwner seems like a safer solution.

r=me with that fixed.

Comment 9

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

Comment on attachment 468984 [details] [diff] [review]
Patch v1.1

>-  if (aParent || HasAttr(kNameSpaceID_None, nsGkAtoms::form)) {
>+  // If @form is set, the element *has* to be in a document, otherwise it
>+  // wouldn't be possible to find an element with the corresponding id.
>+  // If @form isn't set, the element *has* to have a parent, otherwise it
>+  // wouldn't be possible to find a form ancestor.
>+  // We should not call UpdateFormOwner if none of these coniditions are
>+  // respected.

s/respected/fulfilled/

See previous comment regarding the double-call of HasAttr and moving the check to UpdateFormOwner (there really is no need to save on a null-check, just make it part of the isempty check)

r=me with that fixed

Comment 10

[Mounir Lamouri (:mounir)]

(In reply to comment #8)
> if (HasAttr(...) ? !!GetCurrentDoc() : !!aParent)

Why "!!" ?

(In reply to comment #9)
> See previous comment regarding the double-call of HasAttr and moving the check
> to UpdateFormOwner (there really is no need to save on a null-check, just make
> it part of the isempty check)

This can only happen in DEBUG so that doesn't sound really dangerous. The GetCurrentDoc() is only doc to check we got the element we were expecting. Having it called/checked in realease seems useless.
I'm going to change the second assert to prevent any crash in debug.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Because "A ? B : C" won't compile on many compilers if B and C are different types.

s/coniditions/conditions/?

Comment 12

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/a1e65a7730f4

With the type unfortunately...

Comment 13

[Boris Zbarsky [:bzbarsky]]

Well, push a typo fix?  ;)

Comment 14

[Mounir Lamouri (:mounir)]

(In reply to comment #13)
> Well, push a typo fix?  ;)

Only if you give me a r+ :)

Comment 15

[Boris Zbarsky [:bzbarsky]]

For which?  The typo fix? r+