Bug 52366 – ScriptExecutionContext needs additional checks when active dom objects are created/destroyed

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: New Bugs

[Jeremy Orlow]

SerializedScriptValue needs to iterate over a copy of all active dom objects

[Jeremy Orlow]

Created attachment 78812 [details]
Patch

[Alexey Proskuryakov]

A good catch as to why tests are flaky! I'm not yet convinced if making a copy for iteration is the best fix though.

With this kind of protection, we make fewer assumptions about behavior of ActiveDOMObject methods at a certain runtime cost. It becomes possible to remove objects from the HashMap, but it's still impossible to add them. That can be more confusing than the current requirement of no changes to the HashMap.

Hopefully, stopping an IDBRequest doesn't dispatch any events, or the event handler could create new ActiveDOMObjects.

> When the script execution context is being stopped, we need to abort existing transactions.

IDBTransaction::stop() (or maybe suspend()) is the expected place to stop a transaction, which would not cause indirect destruction. Can it work that way?

The existence of two interdependent ActiveDOMObjects (IDBRequest and IDBTransaction) makes things complicated indeed. I'm not quite sure about how exactly they work together though, so I don't have a specific suggestion.

> it's a really messy (if possible at all) to have ActiveDOMObject redirect ref/deref calls properly
> So this patch instead has to double check that each element in the vector is still in the HashMap before dispatching.

That needs at least an assertion that we don't add items to the HashMap while iterating.

[Jeremy Orlow]

(In reply to comment #2)
> A good catch as to why tests are flaky! I'm not yet convinced if making a copy for iteration is the best fix though.
> 
> With this kind of protection, we make fewer assumptions about behavior of ActiveDOMObject methods at a certain runtime cost. It becomes possible to remove objects from the HashMap, but it's still impossible to add them. That can be more confusing than the current requirement of no changes to the HashMap.

It's no different than if the new items were added to the beginning of the list (and thus skipped), but still I guess it could be a little confusing/odd.  I also can't think of why this would happen though.  Presumably anything that's an ActiveDOMObject is started because of some JavaScript call (and returned back to JS land), right?  So really this should never be happening during a stop()/suspend()/etc call...right?
 
> Hopefully, stopping an IDBRequest doesn't dispatch any events, or the event handler could create new ActiveDOMObjects.

All IndexedDB events are fired asynchronously (i.e. from a timer) so I'm pretty sure we don't.
 
> > When the script execution context is being stopped, we need to abort existing transactions.
> 
> IDBTransaction::stop() (or maybe suspend()) is the expected place to stop a transaction, which would not cause indirect destruction. Can it work that way?

The only reason why anything needs to abort the transaction when the document is being destroyed is in the case that more work is going to be queued up in the onsuccess and/or onerror callback of an IDBRequest.  If an IDBRequest object is still alive (and hasn't fired its callback) then it's likely other instructions are still pending.

The IDBTransaction object, on the other hand, only has onabort and oncomplete callbacks which are called _after_ the transaction has finished.  So even if theres a live IDBTransaction object which hasn't fired any of its events, we don't need to abort anything.

...now that I think about it though, with some recent spec changes that haven't yet been implemented, the IDBTransaction probably will need to be reffed by the IDBRequest, which may actually solve this problem.  On the other hand, though, I'm concerned that the current state of ActiveDOMObject is rather fragile and that this problem will come up again in the future (and not be easily diagnosable).  On the other hand, if we had some assert that caught that case, I'd be less worried.

> The existence of two interdependent ActiveDOMObjects (IDBRequest and IDBTransaction) makes things complicated indeed. I'm not quite sure about how exactly they work together though, so I don't have a specific suggestion.

It's kind of complex.  And I don't fully have a handle on it given that we still have some unimplemented changes which will probably change things a bit.  I could try to write it out if it'd be useful though.

> > it's a really messy (if possible at all) to have ActiveDOMObject redirect ref/deref calls properly
> > So this patch instead has to double check that each element in the vector is still in the HashMap before dispatching.
> 
> That needs at least an assertion that we don't add items to the HashMap while iterating.

I agree that, at a minim, that should be done.

[Alexey Proskuryakov]

Comment on attachment 78812 [details]
Patch

It seems that we need to make a practical decision on what to do now. Certainly, flaky tests and other effects of current behavior is no good.

Do you think that implementing the recent check changes can be done soon enough for us to not need this patch?

> I agree that, at a minim, that should be done.

I'm marking this r- to have this done, and probably for some comments explaining the state of code to be added. As mentioned above, it would be better if we didn't have to make the copy, but if there's no other practical way to stop the flakiness soon, we'll have to go for it.

[Jeremy Orlow]

(In reply to comment #4)
> (From update of attachment 78812 [details])
> It seems that we need to make a practical decision on what to do now. Certainly, flaky tests and other effects of current behavior is no good.
> 
> Do you think that implementing the recent check changes can be done soon enough for us to not need this patch?

I've thought about this more and realized that this particular problem will not go away with the resulting spec changes.
 
> > I agree that, at a minim, that should be done.
> 
> I'm marking this r- to have this done, and probably for some comments explaining the state of code to be added. As mentioned above, it would be better if we didn't have to make the copy, but if there's no other practical way to stop the flakiness soon, we'll have to go for it.

As far as I can tell, there are 2 options here:
1) I could just do these changes and then submit this
2) We could scrap this patch and I could instead have all IDBTransaction objects be released with some sort of "ReleaseSoon" class which would set a timer and upon it firing do the release.  This would fix the problem and still break reference cycles properly.

How expensive are adding timers?  If they're pretty cheap, then 2 _might_ have a measurable perf advantage (though we never should have that many active dom objects or suspend/resume/stops, right?).  2 will likely be a bit more fragile, but we could minimize this with asserts.

AP: Any thoughts on which would be best?

[Jeremy Orlow]

ScriptExecutionContext needs additional checks when active dom objects are created/destroyed...to make sure that we don't have any more of these races.

https://bugs.webkit.org/show_bug.cgi?id=52722 and https://bugs.webkit.org/show_bug.cgi?id=52719 are for the actual problems that were caused by not having these checks.

[Jeremy Orlow]

Created attachment 80163 [details]
Patch

[Early Warning System Bot]

Attachment 80163 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7497333

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

Attachment 80163 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7540361

[Hans Wennborg]

Comment on attachment 80163 [details]
Patch

Looks good (except the bot redness) as far as I can tell.

[Jeremy Orlow]

Created attachment 80209 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 80209 [details]
Patch

Since it's a bool and not a counter, please ASSERT(!m_iteratingActiveDOMObjects) before setting it to true.

It's a little unfortunate to incur a release mode cost for a debug-only assertion.

[Jeremy Orlow]

I could make functions and/or a class for it.  With that, I could #ifndef NDEBUG.

[Jeremy Orlow]

Created attachment 80257 [details]
with #ifndef NDEBUG

See if you like this any better.

[Jeremy Orlow]

These bugs have essentially merged.  Sorry for the disorganization...

*** This bug has been marked as a duplicate of bug 52722 ***