Bug 52719

XHR::stop should not call reportExtraMemoryCost() (or should do it more carefully) because it can cause a GC (in at least JSC) "..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)" (Sorry if the bug report is less than ideal, but I don't fully understand what's going on here...) 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