IDBTransaction and IDBRequest can be deleted while ScriptExecutionContext is iterating....which is bad

• Component: New Bugs
• Status: RESOLVED FIXED

[Jeremy Orlow]

IDBRequest and IDBTransaction should not be Active DOM Objects.  We should instead have a single IDB object which has a stop handler.  When this is called, it'll abort all running transactions (which should have registered themselves with it).  It's essential that this object does not cause any other active DOM objects to get killed or run any JavaScript synchronously.

We should only have one of these objects per script execution context.  And when there are no active transactions, it should be destroyed.


Context:

[6:54pm] ap: jorlow: ayt?
[6:54pm] jorlow: ap: what's up?
[6:55pm] ap: jorlow: I'm finally looking at your patch again, and I'm wondering why we even need to stop or abort transactions
[6:55pm] jorlow: so that when you leave a page stuff doesn't keep running
[6:55pm] ap: jorlow: it seems like requests need to be stopped, not transactions
[6:55pm] jorlow: no
[6:55pm] • ap listens
[6:55pm] jorlow: if you leave a page, a transaction can't ever be committed
[6:56pm] jorlow: requests can't be aborted
[6:56pm] jorlow: only transactions
[6:57pm] ap: jorlow: ok. then looking at it from a different angle, why are requests ActiveDOMObjects?
[6:57pm] jorlow: they need to be...it might be a fair question why IDBTransactions are though
[6:57pm] jorlow: requests need to be because if someone is listening to onsuccess or onerror, then it's possible they'll abort the transaciton or schedule another operation in that handler
[6:58pm] jorlow: and that the transaction committed at that point would be a logic error
[6:59pm] jorlow: transactions, on the other hand, only fire their event listeners after abort or complete happens
[6:59pm] jorlow: hm.....ok.....so by my earlier logic, maybe we should be aborting transactions with ::stop either way....  
[6:59pm] jorlow: i.e. if they navigate away, that's a signal they want the app to stop doing work
[6:59pm] jorlow: do event targets need to be active dom objects?
[7:00pm] ap: jorlow: only if nothing else can guarantee that they live long enough
[7:00pm] ap: jorlow: e.g. XMLHttpRequest is an ActiveDOMObject, but XMLHttpRequest.upload is not
[7:01pm] ap: jorlow: because XMLHttpRequest keeps XMLHttpRequestUpload alive
[7:01pm] jorlow: hmm
[7:01pm] ap: jorlow: we just need to pick a central object that will outlive others, and manage their lifetime
[7:01pm] jorlow: they may not need to be active dom objects then
[7:01pm] jorlow: let me think on this some
[7:02pm] jorlow: (and grab dinner...brb)
[7:02pm] ap: jorlow: bon appetit 
[7:05pm] ap: jorlow: (well, manage lifetime and suspend/stop/resume), of course. Since requests belong to transactions, the latter should be able to centralize everything
[7:20pm] jorlow: ap: so the one reason left why we need these to be active dom objects is that we need the page not to go into cache
[7:20pm] jorlow: any suggestions on how else to do that?
[7:20pm] jorlow: ap: er....page go into page cache
[7:21pm] ap: jorlow: either transaction or request probably needs to be an active dom object, right?
[7:21pm] ap: jorlow: otherwise, what would protect them from being destroyed if there are no JS references to them
[7:22pm] jorlow: ap: yeah...make sense
[7:23pm] jorlow: but i think then we're back to a situation where ::stop() can cause the active dom object to be killed
[7:23pm] jorlow: ap: the IDBTransaction needs to be active...not IDBRequest, btw
[7:23pm] jorlow: since active transactions are the reason it can't go to cache
[7:24pm] ap: jorlow: sounds reasonable
[7:24pm] jorlow: but the IDBTransactionBackendImpl needs to have a ref to the IDBTransaction.  And when abort happens, that link is broken
[7:24pm] jorlow: and that can cause IDBTransaction to go to 0 refs
[7:25pm] jorlow: ap^^
[7:28pm] ap: jorlow: hmm... ok, so what happens in XMLHttpRequest case? ScriptExecutionContext::stopActiveDOMObjects() calls XMLHTTPRequest::stop() - so far, the same as with IDB, right?
[7:28pm] ap: jorlow: XMLHTTPRequest::stop() supposedly releases the last ref, and we should be getting into ScriptExecutionContext::destroyedActiveDOMObject()
[7:28pm] jorlow: hmmm
[7:28pm] ap: jorlow: so either we have the same problem with XHR, or there's some witty trick
[7:33pm] ap: jorlow: ah, I know what the trick is
[7:34pm] jorlow: ap: yeah?
[7:34pm] ap: jorlow: XMLHttpRequest always has a JS wrapper, which keeps a reference to it
[7:34pm] jorlow: why?
[7:35pm] ap: jorlow: there is no way to create an XMLHttpRequest without creating a wrapper
[7:35pm] jorlow: But couldn't the wrapper get GCed?
[7:36pm] ap: jorlow: not while there's an ongoing request (that's what ActiveDOMObject is primarily about)
[7:36pm] ap: jorlow: and if there is no request, and the wrapper gets GC'ed, then XMLHttpREquest itself is destroyed, too
[7:37pm] ap: jorlow: now, that doesn't quite work even for XHR
[7:38pm] ap: jorlow: because XHR::stop ends up calling reportExtraMemoryCost() for JSC, and that can collect the unprotected wrapper of another XHR (the current one is still protected at the moment)
[7:39pm] jorlow: ap: ouch..that's subtle
[7:39pm] jorlow: or any other active dom object, for that matter
[7:39pm] ap: jorlow: if there we really no JS code running from stop(), then the extra reference we're looking for would have been guaranteed by the wrapper
[7:40pm] ap: jorlow: but even XHR violates this requirement by forcing GC sometimes
[7:40pm] jorlow: ap: to be honest, i still don't fully understand why...I think maybe I just need to look at the code with these comments in mind (can't at the moment)
[7:40pm] jorlow: but it also seems as though maybe it doesn't matter
[7:40pm] MX80 left the chat room. (Remote host closed the connection)
[7:41pm] jorlow: and that we need a more general solution to the problem
[7:48pm] jorlow: is Helder Correia on irc?
[7:49pm] jorlow: ap would you agree with my last comment?
[7:49pm] jorlow: my thinking is that the subtlety of these 2 issues points to the need for a more general solution....maybe the one I originally proposed
[7:50pm] ap: jorlow: I don't know. Seems easy enough to fix XMLHttpRequest to not force GC from stop(), but it's indeed subtle
[7:50pm] jorlow: I think removing active dom objects during iteration is probably much more likely and less determinisitc than adding
[7:50pm] jorlow: and it kind of seems like it's only a matter of time before we see another one of these
[7:51pm] jorlow: personally, I'd rather go with the general solution...I'm not super happy with the one I presented, but I can't think of a better one
[7:51pm] ap: jorlow: if we go with your original solution, I'd prefer that we don't do the contains() check though
[7:51pm] jorlow: ap: how can we avoid it?
[7:52pm] jorlow: I guess if the vector we iterate over was a member variable we could
[7:52pm] ap: jorlow: having a no-longer-active inactive object be destroyed while stopping further objects is kind of normal indeed
[7:52pm] jorlow: by moving the last element in to replace the removed element
[7:53pm] ap: jorlow: but the fact that stopping one object can indirectly destroy others seems evil
[7:53pm] jorlow: ap: aren't all objects stopped or none?
[7:53pm] jorlow: and why is it evil?
[7:53pm] jorlow: i feel like the IDB case of doing this is pretty legit
[7:53pm] jorlow: even if the XHR one is kinda shady
[7:54pm] ap: jorlow: my thinking is that an active object is fairly independent, and ScriptExecutionContext manages these
[7:54pm] ap: jorlow: that's why there are so few guarantees in this iteration code
[7:55pm] ap: jorlow: having a "sub-manager" inside IDB sounds complicated
[7:55pm] ap: jorlow: and this complicated situation is what we have now - ScriptExecutionContext can stop an active object, but also one IDB active object can stop another one
[7:55pm] jorlow: ap: actually, i was just going to suggest that maybe we should have one active dom object within IDB that they all share
[7:56pm] ap: jorlow: right, and if we do that, we wont need a contains() check, correct?
[7:56pm] jorlow: we wouldn't need anything in my patch actually
[7:56pm] jorlow: if we fix the XHR thing as well
[7:56pm] jorlow: (though we should put in the ASSERTs you suggested)
[7:56pm] ap: jorlow: ok, so how about this plan:
[7:57pm] ap: 1. Fix XHR
[7:57pm] ap: 2. Fix IDB to have one active dom object
[7:57pm] ap: 3. add an assertion
[7:57pm] jorlow: ap: can I file a bug on you for #1?
[7:57pm] ap: 4. consider making a copy in a vector to make this less subtle
[7:57pm] ap: jorlow: sure, that would be great
[7:57pm] jorlow: k...will do
[7:57pm] jorlow: not sure about 4 tho
[7:58pm] jorlow: I feel like the rest of this is a replacement for it
[7:58pm] jorlow: I'll put a nice change log comment in I guess
[7:58pm] ap: jorlow: it says "consider", not "do" 
[7:58pm] jorlow: k
[7:58pm] jorlow: I'll try to summarize these in the bugs
[7:58pm] jorlow: might not get aroudn to filing until tomorrow though

[Alexander Pavlov (apavlov)]

Alexey Proskuryakov is [e-mail] I believe

[Jeremy Orlow]

FWIW, I'm currently writing this patch and I'm finding that it's an awful lot of similar/duplicate code to ActiveDOMObject.  It's really making me think we should just make the changes to ADO itself.

[Jeremy Orlow]

Created attachment 80158 [details]
Patch

[WebKit Review Bot]

Attachment 80158 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7540357

[WebKit Review Bot]

Attachment 80158 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7542347

[Hans Wennborg]

Comment on attachment 80158 [details]
Patch

Did IDBActiveDOMObject.h slip out of this patch? I can't find it, and that seems to be what makes the bots red.

> Source/WebCore/ChangeLog:9
> +        ActiveDOMObject. The former has less implications the latter and iterates

nit: less implications *than* the latter

> Source/WebCore/storage/IDBActiveDOMObject.cpp:2
> + * Copyright (C) 2010 Google Inc. All rights reserved.

nit: 2011?

> Source/WebCore/storage/IDBActiveDOMObject.cpp:13
> + * 3.  Neither the name of Apple Computer, Inc. ("Apple") nor the names of

I thought this should be a two-clause thing nowadays?

[Jeremy Orlow]

(In reply to comment #6)
> (From update of attachment 80158 [details])
> Did IDBActiveDOMObject.h slip out of this patch? I can't find it, and that seems to be what makes the bots red.

oops...will make sure it's added
 
> > Source/WebCore/ChangeLog:9
> > +        ActiveDOMObject. The former has less implications the latter and iterates
> 
> nit: less implications *than* the latter

oops..will fix

> > Source/WebCore/storage/IDBActiveDOMObject.cpp:2
> > + * Copyright (C) 2010 Google Inc. All rights reserved.
> 
> nit: 2011?

k
 
> > Source/WebCore/storage/IDBActiveDOMObject.cpp:13
> > + * 3.  Neither the name of Apple Computer, Inc. ("Apple") nor the names of
> 
> I thought this should be a two-clause thing nowadays?

k...will change to this: http://webkit.org/coding/bsd-license.html

[Jeremy Orlow]

Created attachment 80207 [details]
new

Weird....webkit-patch refuses to upload that file....created a patch manually.

[Jeremy Orlow]

Alexey, can you take a look at the general approach of IDBActiveDOMObject here and give your comments?  As I mentioned, I want to have something in by EOW even if we need to continue iterating after that.

[Jeremy Orlow]

Comment on attachment 80207 [details]
new

I talked to Nate and JamesR and we came up with something that I think will be far superior and not require all the IDBActiveDOMObject stuff.

IDBTransaction will be an ActiveDOMObject.  IDBRequest won't be.  I'll be able to get rid of the self ref for request.  And IDBTransaction can probably use the built in machinery in ActiveDOMObject for that.

Will try to upload something tomorrow.

[Jeremy Orlow]

Ok, after more experimenting and chatting with people, I'm pretty positive of a few things:
* IDBRequest needs to have access to a ScriptExecutionContext* and needs to know when stop is called so it can null out the pointer when it's invalid.
* IDBTransaction needs the same + it needs to abort its transaction on ::stop and disable suspension while transactions are running.
* IDBTransaction and IDBTransactionBackend both depend on each other (well, technically the former implements the IDBTransactionCallbacks interface and the latter actually owns that).  The transaction finishing is, as far as I can tell, the only suitable time to break the reference cycles.  As far as I can tell, the only solution is to allow IDBTransaction to be deleted while the ::stop()s are being called.

So, first of all, how do we allow ::stop() to be called and handle the ScriptExecutionContext* bit?  Well, to me, these are clearly ActiveDOMObjects even if the name of the class needs to be changed.  (If so, please make a suggestion for a new name?  Or we can do it in a later patch.)

Next, assuming we have to "allow IDBTransaction to be deleted while the ::stop()s are being called", I see a couple options:
1) ActiveDOMObject could iterate on a copy.  We do this fairly often.  It shouldn't be too bad perf wise since we should never have too many.
2) We could make the IDBTransactionBackend -> IDBTransaction connection weak.
3) We could switch its HashMap to be some data structure which would handle the iteration fine.
4) We could make HashMap handle such iteration.
5) We could fork ActiveDOMObject and create some base type for things that can be deleted during iteration.  Personally, this seems like premature optimization to me.

Personally I think #1 is best because the rest are making things more difficult than necessary.  But #2 would work and I had actually considered doing it at an earlier point.

Note that https://bug-52366-attachments.webkit.org/attachment.cgi?id=78812 implemented #1.

[Jeremy Orlow]

Oops

[Jeremy Orlow]

+ mihai and jamesr.  If you had any thoughts about comment #11, they'd be greatly appreciated.

[Jeremy Orlow]

Created attachment 80871 [details]
reposting my original solution to this problem

I'm reposting my original solution to the problem as I think it's the best answer.

[Andrei Popescu]

(In reply to comment #11)
> Ok, after more experimenting and chatting with people, I'm pretty positive of a few things:
> * IDBRequest needs to have access to a ScriptExecutionContext* and needs to know when stop is called so it can null out the pointer when it's invalid.
> * IDBTransaction needs the same + it needs to abort its transaction on ::stop and disable suspension while transactions are running.
> * IDBTransaction and IDBTransactionBackend both depend on each other (well, technically the former implements the IDBTransactionCallbacks interface and the latter actually owns that).  The transaction finishing is, as far as I can tell, the only suitable time to break the reference cycles.  As far as I can tell, the only solution is to allow IDBTransaction to be deleted while the ::stop()s are being called.
> 


Maybe stupid question: rather than

http://trac.webkit.org/browser/trunk/Source/WebCore/storage/IDBTransactionBackendImpl.cpp#L126

could we call onAbort asynchronously (e.g. schedule an event or use a timer)? That means that the cycle would be broken outside of the loop in ScriptExecutionContext::stopActiveDOMObjects()

Andrei

[Jeremy Orlow]

(In reply to comment #15)
> (In reply to comment #11)
> > Ok, after more experimenting and chatting with people, I'm pretty positive of a few things:
> > * IDBRequest needs to have access to a ScriptExecutionContext* and needs to know when stop is called so it can null out the pointer when it's invalid.
> > * IDBTransaction needs the same + it needs to abort its transaction on ::stop and disable suspension while transactions are running.
> > * IDBTransaction and IDBTransactionBackend both depend on each other (well, technically the former implements the IDBTransactionCallbacks interface and the latter actually owns that).  The transaction finishing is, as far as I can tell, the only suitable time to break the reference cycles.  As far as I can tell, the only solution is to allow IDBTransaction to be deleted while the ::stop()s are being called.
> > 
> 
> 
> Maybe stupid question: rather than
> 
> http://trac.webkit.org/browser/trunk/Source/WebCore/storage/IDBTransactionBackendImpl.cpp#L126
> 
> could we call onAbort asynchronously (e.g. schedule an event or use a timer)? That means that the cycle would be broken outside of the loop in ScriptExecutionContext::stopActiveDOMObjects()

I'm not sure about that exact location, but having the abort call in IDBTransaction/IDBRequest::stop() seems like a pretty good solution.

[Alexey Proskuryakov]

> * IDBRequest needs to have access to a ScriptExecutionContext* and needs to know when stop is called so it can null out the pointer when it's invalid.

The ScriptExecutionContext pointer is normally zeroed out in contextDestroyed(), not in stop().

[Jeremy Orlow]

(In reply to comment #17)
> > * IDBRequest needs to have access to a ScriptExecutionContext* and needs to know when stop is called so it can null out the pointer when it's invalid.
> 
> The ScriptExecutionContext pointer is normally zeroed out in contextDestroyed(), not in stop().

Oops!  Good catch.  I guess I'll add a flag to keep track of it then.

[Jeremy Orlow]

(In reply to comment #18)
> (In reply to comment #17)
> > > * IDBRequest needs to have access to a ScriptExecutionContext* and needs to know when stop is called so it can null out the pointer when it's invalid.
> > 
> > The ScriptExecutionContext pointer is normally zeroed out in contextDestroyed(), not in stop().
> 
> Oops!  Good catch.  I guess I'll add a flag to keep track of it then.

Er...I thought this was the other bug where I have code that assumes this.

Anyway, you're right that it's contextDestroyed not stop, but I don't think that actually changes anything in this case.


......hm....

Actually, can't we just do what Geoffrey suggested in https://bugs.webkit.org/show_bug.cgi?id=52719#c5 in contextDestroyed and then do the work that could lead to destruction in contextDestroyed?  It seems like this should be permissible even if you're going to argue that destruction of ActiveDOMObjects during the other points of iteration isn't.

Thoughts?

[Alexey Proskuryakov]

> can't we just do what Geoffrey suggested in https://bugs.webkit.org/show_bug.cgi?id=52719#c5 in contextDestroyed

Sounds good to me, as long as we have an assertion that new ActiveDOMObjects aren't added during contextDestroyed() iteration. Again, if we could have an assertion that no JS is executed during the iteration, that would be better yet. Maybe ScriptController is already dysfunctional at this point?

[Jeremy Orlow]

*** Bug 52366 has been marked as a duplicate of this bug. ***

[Jeremy Orlow]

Created attachment 80952 [details]
Patch

[Nate Chapin]

Comment on attachment 80952 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=80952&action=review

> Source/WebCore/dom/ScriptExecutionContext.cpp:261
> +    ASSERT(!m_iteratingActiveDOMObjects);
> +    ASSERT(!m_inDestructor);

Here and elsewhere: Should we be doing something more than just ASSERTing in debug?  This seems like it'll be a security problem if we do ever hit this case in release builds.

LGTM otherwise.

[Jeremy Orlow]

(In reply to comment #23)
> (From update of attachment 80952 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80952&action=review
> 
> > Source/WebCore/dom/ScriptExecutionContext.cpp:261
> > +    ASSERT(!m_iteratingActiveDOMObjects);
> > +    ASSERT(!m_inDestructor);
> 
> Here and elsewhere: Should we be doing something more than just ASSERTing in debug?  This seems like it'll be a security problem if we do ever hit this case in release builds.
> 
> LGTM otherwise.

Good point!  Although this should never happen in theory, if it did, it could turn into a security vulnerability (due to the dangling ScriptExecutionContext pointer).  It seems there's really nothing good to do in such a case, so we should probably just crash in such a case.  Will make the change.

[Jeremy Orlow]

Created attachment 80984 [details]
Patch

[Jeremy Orlow]

Created attachment 80990 [details]
Patch

[Nate Chapin]

Comment on attachment 80990 [details]
Patch

Ok.

Is it worth an explanatory comment as to why we're crashing instead of just asserting?

[Jeremy Orlow]

Comment on attachment 80990 [details]
Patch

If someone wants to remove it, I'd hope they'd look at the change that added it and thus see this thread.  That seems to be the WebKit way anyhow...for better or worse.

[WebKit Commit Bot]

Comment on attachment 80990 [details]
Patch

Clearing flags on attachment: 80990

Committed r77456: <http://trac.webkit.org/changeset/77456>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.