Need safe way to iterate a document's presshells

RESOLVED FIXED

Status

()

Core
DOM
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: bz, Assigned: smaug)

Tracking

Trunk
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 -
wanted1.9 +
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

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...
Flags: blocking1.9?
Depends on: 378780
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?
Assignee: general → Olli.Pettay
Flags: blocking1.9? → blocking1.9-
Whiteboard: [wanted-1.9]
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...
Status: NEW → ASSIGNED
Created attachment 264166 [details] [diff] [review]
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.
Attachment #264166 - Flags: review?(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.
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?

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.

The other option is to make sure that GetShellAt is used in a safe way.
> 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.
Created attachment 264268 [details] [diff] [review]
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?
Attachment #264166 - Attachment is obsolete: true
Attachment #264166 - Flags: review?(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...
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.
Created attachment 264353 [details] [diff] [review]
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.
Attachment #264268 - Attachment is obsolete: true
Attachment #264353 - Flags: superreview?(bzbarsky)
Attachment #264353 - Flags: review?(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.
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?
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() {}
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...
Depends on: 380674
Created attachment 265685 [details] [diff] [review]
nsPresShellIterator only in stack

This needs bug 380674.
Attachment #265685 - Flags: superreview?(bzbarsky)
Attachment #265685 - Flags: review?(bzbarsky)
Attachment #264353 - Attachment is obsolete: true
Attachment #264353 - Flags: superreview?(bzbarsky)
Attachment #264353 - Flags: review?(bzbarsky)
Attachment #265685 - Flags: superreview?(jonas)
Attachment #265685 - Flags: superreview?(bzbarsky)
Attachment #265685 - Flags: review?(jonas)
Attachment #265685 - Flags: review?(bzbarsky)
Comment on attachment 265685 [details] [diff] [review]
nsPresShellIterator only in stack

This looks great.  Thank you for doing it!

r+sr=bzbarsky
Attachment #265685 - Flags: superreview?(jonas)
Attachment #265685 - Flags: superreview+
Attachment #265685 - Flags: review?(jonas)
Attachment #265685 - Flags: review+
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 19

11 years ago
Created attachment 265818 [details] [diff] [review]
Remove more uses of GetNumberofShells()
Attachment #265818 - Flags: review?(Olli.Pettay)
Comment on attachment 265818 [details] [diff] [review]
Remove more uses of GetNumberofShells()

Sorry, I should have notice these.
Attachment #265818 - Flags: review?(Olli.Pettay) → review+

Comment 21

11 years ago
Checked in the follow-up patch.
Flags: in-testsuite-
Flags: wanted1.9+
Whiteboard: [wanted-1.9]
You need to log in before you can comment on or make changes to this bug.