346083 - Crash [@ nsBoxObject::SetProperty] passing undefined to setProperty

Status: VERIFIED FIXED

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

Description

[Jesse Ruderman]

Passing undefined to boxObj.setProperty causes a crash [@ nsBoxObject::SetProperty].

This is similar to bug 327776, so the fix will probably be similar.

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Attachment: proposed patch

I think removing property when undefined or null is used as value makes sense.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 230885 [details] [diff] [review]
proposed patch

I don't like the magic "read your mind" thing with removing.  Not only that, but that would make this inconsistent with SetPropertyAsSupports.

Why not just set to a null PRUnichar or to EmptyString.get() or something?

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Null PRUnichar makes nsDependentString to crash and EmptyString.get() != null.
I want that if someone uses setProperty('foo', null)
getProperty('foo') returns null, not empty string.

But new patch coming.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

or no (new patch). I'd have to change nsPresState::GetStateProperty too.
It returns NS_STATE_PROPERTY_NOT_THERE/null if the property is either null
or doesn't exist.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Last I checked, we wanted to stop using nsPresState here anyway.  If you can store null for now and just deal with the nsDependentString issue, we can fix the behavior when we don't use nsPresState, I guess...

roc may have opinions; he's looked at this code more than I recently.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Attachment: store null strings

This at least should do the right thing - storing null strings as properties.
(I should file a bug so that IsVoid flags is copied automatically when converting strings)

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 230973 [details] [diff] [review]
store null strings

>Index: layout/base/nsPresState.cpp
>+  nsCAutoString data = NS_ConvertUTF16toUTF8(aValue);
>+  data.SetIsVoid(aValue.IsVoid());

How about:

  NS_ConvertUTF16toUTF8 data(aValue);
  data.SetIsVoid(aValue.IsVoid());

Rest looks good.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Attachment: for check in

Comment 10

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using https://bugzilla.mozilla.org/attachment.cgi?id=230883&action=view as the testcase with build 2006-07-28-07 of SeaMonkey trunk under Windows XP; no crash.

Comment 11

[Jesse Ruderman]

Crashtest checked in.