1659955 - Crash in [@ OOM | large | mozalloc_abort | moz_xmalloc | nsTArray_base<T>::EnsureCapacity<T> | nsTArray_Impl<T>::ReplaceElementsAtInternal<T> | nsObserverList::FillObserverArray]

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Simon Giesecke [:sg] [he/him]]

This bug is for crash report bp-4caeb001-8fba-48d0-b4d7-f82420200818.

Top 10 frames of crashing thread:

0 mozglue.dll mozalloc_abort memory/mozalloc/mozalloc_abort.cpp:33
1 mozglue.dll mozalloc_handle_oom memory/mozalloc/mozalloc_oom.cpp:51
2 mozglue.dll moz_xmalloc memory/mozalloc/mozalloc.cpp:54
3 xul.dll nsTArray_base<nsTArrayInfallibleAllocator, nsTArray_RelocateUsingMemutils>::EnsureCapacity<nsTArrayInfallibleAllocator> xpcom/ds/nsTArray-inl.h:162
4 xul.dll nsTArray_Impl<nsMaybeWeakPtr<nsIObserver>, nsTArrayInfallibleAllocator>::ReplaceElementsAtInternal<nsTArrayInfallibleAllocator, nsMaybeWeakPtr<nsIObserver> > xpcom/ds/nsTArray.h:2356
5 xul.dll nsObserverList::FillObserverArray xpcom/ds/nsObserverList.cpp:35
6 xul.dll nsObserverList::NotifyObservers xpcom/ds/nsObserverList.cpp:62
7 xul.dll nsObserverService::NotifyObservers xpcom/ds/nsObserverService.cpp:288
8 xul.dll mozilla::dom::ContentChild::RecvFlushMemory dom/ipc/ContentChild.cpp:2483
9 xul.dll mozilla::dom::PContentChild::OnMessageReceived ipc/ipdl/PContentChild.cpp:10379

It's not clear to me why nsObserverList::FillObserverArray needs to make a copy of mObservers at all. The algorithm already removes elements from the end, and uses the indexing operator, so it should be safe against the removals while looping.

Obviously, SetCapacity may already fail as well. In case there are a lot of expired weak references, this could be avoided by iterating through observers first to remove the obsolete ones, then allocate only an array for the remaining ones. Not sure if that's worth it, but we might provide a generic function that does (in pseudocode):

nsTArray<T> FillArrayWithCleanup(nsTArray<T> &src, Predicate predicate) {
  nsTArray<T> res;

 if (!res.SetCapacity(src.Length(), fallible)) {
    // Loop through src and remove all elements matching predicate

     res.SetCapacity(src.Length());

     res = src;
 } else {
       // Loop through src and remove all elements matching predicate, add others to res
 }

  return res;
}

Comment 1

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Simon Giesecke [:sg] [he/him] from comment #0)

It's not clear to me why nsObserverList::FillObserverArray needs to make a copy of mObservers at all. The algorithm already removes elements from the end, and uses the indexing operator, so it should be safe against the removals while looping.

That's because we guarantee to call all the observers in the array, even though some may be removed while iterating over the array. The observer service had some other odd guarantees that slowly went away in the past so we might drop this one too but we should be wary of breakage. See bug 1464781 for an attempt I made at cleaning this up.

Comment 2

[Andrew McCreight [:mccr8]]

The available page file is low in these crashes, so if we improved this we'd probably crash somewhere else. A lot of the OOM sizes are under 1MB, so we'd probably crash the next time we allocated a new JS chunk, which is about 1MB.

If these collisions are rare, you could imagine some kind of copy on write setup. Also, using a segmented array might help. But the volume is pretty low here, and the allocation sizes aren't too ridiculous, so it doesn't seem like it is worthwhile.

Comment 3

[Simon Giesecke [:sg] [he/him]]

(In reply to Gabriele Svelto [:gsvelto] from comment #1)

(In reply to Simon Giesecke [:sg] [he/him] from comment #0)

It's not clear to me why nsObserverList::FillObserverArray needs to make a copy of mObservers at all. The algorithm already removes elements from the end, and uses the indexing operator, so it should be safe against the removals while looping.

That's because we guarantee to call all the observers in the array, even though some may be removed while iterating over the array. The observer service had some other odd guarantees that slowly went away in the past so we might drop this one too but we should be wary of breakage. See bug 1464781 for an attempt I made at cleaning this up.

After reading my comment again, I think it's impossible to read from that what I wanted to say. Sorry for that.

I think you are saying that the callers of FillObserverArray need it to make one copy of mObservers, because when they are iterating over the observers, observers might get unregistered. That's clear so far. What I was trying to say was that I don't see the need that FillObserverArray makes another copy of mObservers (at https://searchfox.org/mozilla-central/rev/6cc48251bb97600fdf11a5b4c5f621bfc8606d55/xpcom/ds/nsObserverList.cpp#35), just to fill the output parameter array and remove expired weak references. AFAIU removing the expired weak references cannot trigger other changes to the array.

So without caring for an OOM on SetCapacity, why not change it to:

void nsObserverList::FillObserverArray(nsCOMArray<nsIObserver>& aArray) {
  aArray.SetCapacity(mObservers.Length());

  for (int32_t i = mObservers.Length() - 1; i >= 0; --i) {
    nsCOMPtr<nsIObserver> observer = mObservers[i].GetValue();
    if (observer) {
      aArray.AppendObject(observer);
    } else {
      // the object has gone away, remove the weakref
      mObservers.RemoveElementAt(i);
    }
  }
}

Or maybe you understood what I meant despite my bad explanation, and I am missing something :)

(In reply to Andrew McCreight [:mccr8] from comment #2)

The available page file is low in these crashes, so if we improved this we'd probably crash somewhere else. A lot of the OOM sizes are under 1MB, so we'd probably crash the next time we allocated a new JS chunk, which is about 1MB.

If these collisions are rare, you could imagine some kind of copy on write setup. Also, using a segmented array might help. But the volume is pretty low here, and the allocation sizes aren't too ridiculous, so it doesn't seem like it is worthwhile.

Yes, probably not worth dealing for the OOM itself, but maybe to avoid unnecessary copies in general. If my assumption above holds.

Comment 4

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1659955 - Avoid double copy of mObservers in FillObserverArray. r=#xpcom-reviewers,gsvelto

Comment 5

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1659955 - Replace output parameter by return value in FillObserverArray and rename accordingly. r=#xpcom-reviewers

Depends on D88108

Comment 6

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1659955 - Fix leak counting in nsObserverList. r=#xpcom-reviewers

Depends on D88109

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c5cb9cd7cce3
Avoid double copy of mObservers in FillObserverArray. r=gsvelto,froydnj
https://hg.mozilla.org/integration/autoland/rev/aa3ecf5b9d45
Replace output parameter by return value in FillObserverArray and rename accordingly. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/1415fd1b5052
Fix leak counting in nsObserverList. r=froydnj

Comment 8

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/c5cb9cd7cce3
https://hg.mozilla.org/mozilla-central/rev/aa3ecf5b9d45
https://hg.mozilla.org/mozilla-central/rev/1415fd1b5052