Closed Bug 590387 Opened 9 years ago Closed 9 years ago

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

Categories

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

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla2.0b5
Tracking Status
blocking2.0 --- beta7+

People

(Reporter: jruderman, Assigned: mounir)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, crash, testcase)

Crash Data

Attachments

(4 files)

###!!! 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]
I think web pages and bookmarklets are somewhat likely to hit this bug.
blocking2.0: --- → ?
Attached file stacks
The assumption that aBindToTree implies non-null current doc in UpdateFormOwner seems bogus to me.
Marking blocking beta6, but it'd be nice to fix this asap.
blocking2.0: ? → beta6+
Assignee: nobody → mounir.lamouri
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
Summary: Crash [@ nsGenericHTMLFormElement::UpdateFormOwner] with HTML5 <output form> → Crash [@ nsGenericHTMLFormElement::UpdateFormOwner] when attaching an element with @form set to an element which is not in a document
Attached patch Patch v1Splinter Review
Attachment #468981 - Flags: review?(jonas)
I can't quite figure out what the comments mean to say....
Attached patch Patch v1.1Splinter Review
Trying to improving the comment.
Attachment #468981 - Attachment is obsolete: true
Attachment #468984 - Flags: review?(jonas)
Attachment #468981 - Flags: review?(jonas)
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.
Attachment #468981 - Attachment is obsolete: false
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
Attachment #468984 - Flags: review?(jonas) → review+
(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.
Because "A ? B : C" won't compile on many compilers if B and C are different types.

s/coniditions/conditions/?
Pushed:
http://hg.mozilla.org/mozilla-central/rev/a1e65a7730f4

With the type unfortunately...
Well, push a typo fix?  ;)
(In reply to comment #13)
> Well, push a typo fix?  ;)

Only if you give me a r+ :)
For which?  The typo fix? r+
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b5
Crash Signature: [@ nsGenericHTMLFormElement::UpdateFormOwner]
You need to log in before you can comment on or make changes to this bug.