378776 - Need safe way to iterate a document's presshells

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

The current method of iterating presshells is unsafe, because so many operations on presshells can cause them to go away.  If there happens to be more than one presshell associated with the document at the time, this means that we might run off the end of the presshell array and crash due to (best case) a null pointer deref or (worst case) a virtual call on a garbage pointer.

Ideally we'd just use nsTObserverArray here...

Comment 1

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

Not a blocker, but would be great if we could fix this. Should be easy enough.

Smaug: Do you have the cycles to fix this one?

Comment 2

[Boris Zbarsky [:bzbarsky]]

So what I've been thinking, once bug 378780 is fixed, is that we'd have GetPrimaryShell(), and for enumeration we'd have EnumerateShells() which takes a method and a closure.  The method would take an nsIPresShell* (from a strong ref held by EnumerateShells() itself) and the closure passed to EnumerateShells().

Alternately we could implement a copy-ctor for nsTObserverArray iterators and have a method that gets an iterator for the presshell array, I guess...

Comment 3

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

Attachment: possible patch

This is pretty simple API, and as an example I converted the cases 
where events are dispatched while iterating through the shells.
(One could argue whether it makes sense to dispatch events to all prescontexts but that is a different bug)

The old API can be used when there isn't anything unsafe happening.
Not sure about what nsContentSink should use, probably the new safe API.

Comment 4

[Boris Zbarsky [:bzbarsky]]

I think we should just eliminate the old API.  We have too many things that are possibly "unsafe"...  The content sink absolutely must use the new API: what it does can run arbitrary script.

Will look at the patch in a bit.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Why bother dealing with destroyed shells, though?  I guess this might be simpler than trying to deal with a live array, and in the common case (one presshell) there's really not that much difference here...  Need to think about it a bit.

How amenable are our consumers to moving to this method?

Comment 6

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

How could the shell be iterated then? Maybe just from Count() - 1 to 0.

Converting current GetShellAt(i) callers to use the new method
should be easy.

I'll make a new patch which removes GetShellAt() and probably also
GetNumberOfShells(). That way callers are forced to use EnumerateShells.

Comment 7

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

The other option is to make sure that GetShellAt is used in a safe way.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> How could the shell be iterated then? Maybe just from Count() - 1 to 0.

That's not good enough to handle arbitrary script.  The simple way to avoid dealing with presshells that are no longer relevant, imo, is to use an nsTObserverArray for the preshell storage.  In fact, that could be combined with your API, now that I think about it.

> The other option is to make sure that GetShellAt is used in a safe way.

I think that's very hard to do, and more importantly is fragile.

Comment 9

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

Attachment: Use only EnumerateShells / GetPrimaryShell

This is not a too small patch :( though, it is mainly just moving code 
to nsPresShellEnumerator implementations. (Unfortunately there are quite 
many implementations).
The enumeration API itself is pretty much as simple as it can get.

Any better ideas how the enumeration could be done?

Comment 10

[Boris Zbarsky [:bzbarsky]]

I still think having a callback method and a closure would be more efficient -- no virtual calls, no need to copy data, etc.  Maybe I'm over-engineering...

Comment 11

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

The API isn't used in any performance critical places.
And the only real difference is virtual vs. callback.
Copying data is, I'd say, more up to how an nsPresShellEnumerator
is implemented.

But, what if there would be an iterator for presshells.
Internally it would be nsTObserverArray<nsIPresShell>::ForwardIterator
but it could change the GetNext method to return already_AddRefed<nsIPresShell>. That would force callers to use strong refs to presshells when iterating. I think I'll look at the iterator approach some more.

Comment 12

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

Attachment: nsPresShellIterator

I prefer this approach. Iterators are allocated in heap, but they
aren't in any critical path, as far as I see, and one |new| isn't that
expensive.

Comment 13

[Boris Zbarsky [:bzbarsky]]

I pretty much like that.  Let me think a bit about how we could avoid heap-allocation here, because I hate the OOM checks.... That was the one thing that led me to suggest a callback mechanism to start with; I prefer iteration as well.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Could something like this work?

class nsPresShellIterator : 
  private nsTObserverArray<nsIPresShell>::ForwardIterator {

  // Let nsTObserverArray<nsIPresShell> see through our inheritance
  friend class nsTObserverArray<nsIPresShell>;

  nsPresShellIterator(nsIDocument* aDocument) :
   nsTObserverArray<nsIPresShell>::ForwardIterator(aDocument->mPresShells),
   mDocument(aDocument) {}

  nsIPresshell* GetNext() {
    ...
  }
};

and then just have consumers instantiate one of these on the stack.  This will need to be a friend class for nsIDocument, but that's ok.  You might also need to push the presshell array up to nsIDocument, but I still think that's ok.

Thoughts?

Comment 15

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

But nsTObserverArray isn't public, so I'd have to move it from
content/base/src to some other place.

and that should be already_AddRefed<nsIPresShell> GetNext() {}

Comment 16

[Boris Zbarsky [:bzbarsky]]

We've been meaning to move nsTObserverArray to xpcom/ in any case; we could use it on other places (e.g. the prefservice) that iterate observers.

And yes, on signature.  And if it's out-of-lined it'll need to be virtual, of course...

Comment 17

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

Attachment: nsPresShellIterator only in stack

This needs bug 380674.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 265685 [details] [diff] [review]
nsPresShellIterator only in stack

This looks great.  Thank you for doing it!

r+sr=bzbarsky

Comment 19

[Aaron Leventhal]

Attachment: Remove more uses of GetNumberofShells()

Comment 20

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

Comment on attachment 265818 [details] [diff] [review]
Remove more uses of GetNumberofShells()

Sorry, I should have notice these.

Comment 21

[Aaron Leventhal]

Checked in the follow-up patch.