1229458 - "Assertion failure: !IsReadonly() (shared StringBuffer in SizeOfIncludingThisMustBeUnshared)" with <object id>

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

Attachment: testcase

1. Load the testcase
2. Quickly click "Measure" in about:memory

Assertion failure: !IsReadonly() (shared StringBuffer in SizeOfIncludingThisMustBeUnshared), at xpcom/string/nsSubstring.cpp:323

This assertion was upgraded to fatal in bug 1225365.

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Boris Zbarsky [:bzbarsky]]

Yeah, nsStringHashKey::SizeOfExcludingThis shouldn't be assuming its string is unshared.  It even has a comment about how it shouldn't be assuming it...

Looks like a regression from bug 1046281 and affects some of the other hashkeys too.

Comment 3

[Andrew McCreight [:mccr8]]

Honestly, I'm not really sure of the value of ever using SizeOfIncludingThisMustBeUnshared() instead of SizeOfIncludingThisIfUnshared().

Comment 4

[Andrew McCreight [:mccr8]]

What do you think about just ditching MustBeUnshared() for IfUnshared(), Nick?

Comment 5

[Andrew McCreight [:mccr8]]

The former landed in bug 671299, and the latter in bug 712835.

Comment 6

[Nicholas Nethercote [inactive]]

> What do you think about just ditching MustBeUnshared() for IfUnshared(), Nick?

Yeah, good idea. It's just adding unnecessary brittleness.

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: Remove SizeOfIncludingThisMustBeUnshared() from string classes

The patch changes all uses of SizeOfIncludingThisMustBeUnshared() to
SizeOfIncludingThisIfUnshared(). This incurs the (tiny) cost of an unnecessary
IsReadonly() check for guaranteed-unshared strings, but avoids the possible
assertion failures that would occur when MustBeUnshared() was used incorrectly
on shared strings, which is an easy mistake to make.

Comment 8

[Andrew McCreight [:mccr8]]

Comment on attachment 8694444 [details] [diff] [review]
Remove SizeOfIncludingThisMustBeUnshared() from string classes

Review of attachment 8694444 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for fixing this.

::: xpcom/string/nsTSubstring.cpp
@@ +1005,5 @@
>      mozilla::MallocSizeOf aMallocSizeOf) const
>  {
>    if (mFlags & F_SHARED) {
>      return nsStringBuffer::FromData(mData)->
> +           SizeOfIncludingThisIfUnshared(aMallocSizeOf);

This new indentation looks weird to me. Maybe more like the old one?

Comment 9

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d746378ad672eda7c9f33a0e93e08291bff5f5e3
Bug 1229458 - Remove SizeOfIncludingThisMustBeUnshared() from string classes. r=mccr8.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d746378ad672