| Summary: | IndexedDB tests leak documents | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Simon Fraser (smfr) <simon.fraser> | ||||||||
| Component: | WebKit Misc. | Assignee: | youenn fablet <youennf> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | alecflett, ap, beidson, cdumez, commit-queue, darin, esprehn+autocc, ews-watchlist, ggaren, hi, joepeck, jsbell, kondapallykalyan, sihui_liu, simon.fraser, webkit-bug-importer, youennf | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Simon Fraser (smfr)
2018-09-07 14:51:26 PDT
Created attachment 361349 [details]
Patch
Comment on attachment 361349 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=361349&action=review > Source/WebCore/Modules/indexeddb/IDBCursor.h:109 > + JSValueInWrappedObject m_cachedValue; The patch is going from JSC::String to JSC::Weak. The patch is probably making sure that as long as the JSIDBCursor is alive, we will not GC these values. Can there be cases where: - we create a JSIDBCursor from an IDBCursor, GC the JSIDBCursor and recreate a new JSIDBCursor from the same IDBCursor. - we want to keep the cached values from being GCed when the JSIDBCursor is no longer there. Using DOMGuardedObject might be a solution so that we ensure that these values do not get GCed while making sure we do not create unbreakable ref cycles. Created attachment 361403 [details]
Patch
(In reply to youenn fablet from comment #4) > Comment on attachment 361349 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=361349&action=review > > > Source/WebCore/Modules/indexeddb/IDBCursor.h:109 > > + JSValueInWrappedObject m_cachedValue; > > The patch is going from JSC::String to JSC::Weak. > The patch is probably making sure that as long as the JSIDBCursor is alive, > we will not GC these values. > Can there be cases where: > - we create a JSIDBCursor from an IDBCursor, GC the JSIDBCursor and recreate > a new JSIDBCursor from the same IDBCursor. This is possible. Why is this a problem? > - we want to keep the cached values from being GCed when the JSIDBCursor is > no longer there. If there is JSIDBRequest, we will keep the cached value from being GCed. Other than that, do we need to keep the cached values around without having JSIDBCursor? This patch keeps previous behavior. > If there is JSIDBRequest, we will keep the cached value from being GCed. > Other than that, do we need to keep the cached values around without having > JSIDBCursor? That is my question, yes. I am not familiar enough in IDB to know whether this is ok or not. Code inspection or tests exercising IDB API and GC might help. > This patch keeps previous behavior. Not really, the previous behavior with JSC::Strong is to tie cached values lifetime with IDBCursor lifetime. This patch is tieing these cached values lifetime with JSIDBCursor lifetime. (In reply to youenn fablet from comment #7) > > If there is JSIDBRequest, we will keep the cached value from being GCed. > > Other than that, do we need to keep the cached values around without having > > JSIDBCursor? > > That is my question, yes. > I am not familiar enough in IDB to know whether this is ok or not. > Code inspection or tests exercising IDB API and GC might help. > The change doesn't break any existing test, so far. > > This patch keeps previous behavior. > > Not really, the previous behavior with JSC::Strong is to tie cached values > lifetime with IDBCursor lifetime. This patch is tieing these cached values > lifetime with JSIDBCursor lifetime. Yes, by "keeping previous behavior" I mean keeping expected behavior from layout tests. Tying cached values to IDBCursor caused unexpected document leaks. Comment on attachment 361403 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=361403&action=review r=me > Source/WebCore/Modules/indexeddb/IDBCursor.cpp:131 > + JSValue key = m_cachedPrimaryKey ? JSValue(m_cachedPrimaryKey) : primaryKey(state); JSValueInWrappedObject's operator JSC::JSValue() is not explicit, so I think you can change JSValue(m_cachedPrimaryKey) to just m_cachedPrimaryKey. > Source/WebCore/Modules/indexeddb/IDBCursor.cpp:303 > + JSValue key = m_cachedPrimaryKey ? JSValue(m_cachedPrimaryKey) : primaryKey(state); Ditto. > Source/WebCore/Modules/indexeddb/IDBCursor.cpp:358 > +JSValue IDBCursor::key(ExecState& state) > +{ > + return toJS(state, *state.lexicalGlobalObject(), m_currentKey.get()); > +} > + > +JSValue IDBCursor::primaryKey(ExecState& state) > +{ > + return toJS(state, *state.lexicalGlobalObject(), m_currentPrimaryKey.get()); > +} > + > +JSC::JSValue IDBCursor::value(ExecState& state) > +{ > + return deserializeIDBValueToJSValue(state, m_currentValue); > +} We usually put these wrapper functions that call toJS() into our JSXXX.cpp or JSXXXCustom.cpp files. We believe these functions are only invoked by JavaScript bindings code. And if we autogenerated these functions, the autogenerator would put them in the JSXXX.cpp file. So, I'd suggest removing these functions from the IDBCursor class, and moving their code into JSIDBCursorCustom.cpp. In general, it is "bad code smell" to see significant use of JavaScript interfaces inside a C++ DOM object. We usually think of JavaScript as something that wraps C++ DOM objects, and is not part of their internal implementation. Once JavaScript interfaces become part of the internal implementation of a C++ DOM object, we risk the kind of memory leak you're fixing here. > Source/WebCore/Modules/indexeddb/IDBCursor.h:63 > + JSValueInWrappedObject& cachedKey() { return m_cachedKey; } > + JSValueInWrappedObject& cachedPrimaryKey() { return m_cachedPrimaryKey; } > + JSValueInWrappedObject& cachedValue() { return m_cachedValue; } I would call these currentKeyWrapper, currentPrimaryKeyWrapper, and currentValueWrapper -- because these data members/accessors are always null or JavaScript wrappers for currentKey, currentPrimaryKey, and currentValue. Maybe we can also remove the word "current" from all of these names. Any state, at the moment you examine it, is current. > Source/WebCore/Modules/indexeddb/IDBCursor.idl:35 > + [CustomGetter] readonly attribute any key; > + [CustomGetter] readonly attribute any primaryKey; It would be nice, in a future patch, to teach our auto generator how to generate a getter for a JSValueInWrappedObject automatically. But it's OK that these are custom for now. > Source/WebCore/Modules/indexeddb/IDBRequest.h:63 > + enum class OtherResultType { Maybe we should call this NullResultType? Empty and Undefined are different ways to express absence, which we usually call "null". > Source/WebCore/Modules/indexeddb/IDBRequest.h:80 > + JSValueInWrappedObject& cachedResult() { return m_cachedResult; } I would call this resultWrapper. One thing I don't like about the word "cache" is that a cache is usually an optimization that you can delete at any time without changing correctness. But in our case, once we allocate our result and its JS wrapper, it is essential for correctness that they remain in sync. > Source/WebCore/Modules/indexeddb/IDBRequest.h:-83 > - Please revert whitespace change to keep svn history clean. > > Can there be cases where: > > - we create a JSIDBCursor from an IDBCursor, GC the JSIDBCursor and recreate > > a new JSIDBCursor from the same IDBCursor. > > This is possible. Why is this a problem? Can you give an example where this is possible? If this is possible, it is a subtle bug. A JavaScript programmer who set a custom JavaScript property on the IDBCursor, or who put the IDBCursor in a WeakMap, would observe that the property or map entry disappeared when the JSIDBCursor was garbage collected. The likely solution to this bug is to make sure that the other DOM object(s) that provide APIs that return this IDBCursor should, in their custom visit functions, make sure to visit this IDBCursor. If there are many objects that can return this IDBCursor through many different APIs, it might not be practical to deploy custom visit functions for all those casdes, and we'll have to consider other options, probably using the opaque roots API. > > - we want to keep the cached values from being GCed when the JSIDBCursor is > > no longer there. > > If there is JSIDBRequest, we will keep the cached value from being GCed. > Other than that, do we need to keep the cached values around without having > JSIDBCursor? This patch keeps previous behavior. The answer to this question depends on whether DOM objects other than JSIDBRequest offer APIs that return this IDBCursor. If JSIDBRequest is the only object that offers API that returns this IDBCursor, and JSIDBRequest has a custom visit function that takes care to visit this IDBCursor, that's good enough. I don't think DOMGuardedObject is the right solution in this case because it would cause every IDB request and cursor to survive as long as the webpage survives. A webpage that executes lots of IDB queries will see a serious memory leak. In fact, it seems likely to me that many uses of DOMGuardedObject are memory leaks. Consider DOMGuardedObject, I do think I see a pre-existing bug here, which I'd like you to fix in a separate bug: JSValueInWrappedObject neglects to execute any write barrier. JSValueInWrappedObject needs to tell the garbage collector that its owner now points to its value. Otherwise, the garbage collector, if it has already concurrently or generationally visited the owner, may neglect to visit the value. This is probably as simple as: (1) Call heap.writeBarrier(), passing owner and value, inside cachedPropertyValue, after we assign to cachedValue. (2) Change the JSValueInWrappedObject(JSC::JSValue value) to require an owner argument, and execute a write barrier there too. Created attachment 361483 [details]
Patch
Comment on attachment 361483 [details]
Patch
r=me
Comment on attachment 361483 [details] Patch Clearing flags on attachment: 361483 Committed r241196: <https://trac.webkit.org/changeset/241196> All reviewed patches have been landed. Closing bug. I don’t see any tests here or any test results changing. We need to create regression tests that cover the object lifetime, or be clearer about why that is difficult or impossible. When I created JSValueInWrappedObject, I created the reference-cycle-leaks test <https://trac.webkit.org/browser/webkit/trunk/LayoutTests/fast/dom/reference-cycle-leaks.html> and in writing the test was in fact more difficult than fixing the bug. There is a placeholder in there for an IDBRequest test, although not a placeholder for an IDBCursor test. I think it’s dangerous for us to say this is fixed without also adding a regression test, so we should settle for that only if we find it’s prohibitively difficult to do so. Also, since the comments above point to something wrong about the JSValueInWrappedObject implementation or possibly in how it’s being used, I also would like us to create tests that demonstrate that. We could do that either when we fix the problem, or, better, even before we fix the problem to help us understand even more deeply that the problem is real. I think seeing these indirectly through [ Leak ] isn’t the best way to test, but I suppose it’s better than nothing. Seems like we could regression test this in a direct way by following the model of <https://trac.webkit.org/browser/webkit/trunk/LayoutTests/fast/dom/reference-cycle-leaks.html>: 1. Make a test with an iframe 2. Record window.internals.numberOfLiveDocuments() 3. Do 100 times: 3a. Navigate the iframe to a test page that uses IDBCursor 4. Record window.internals.numberOfLiveDocuments() - (2) Write barrier test will be hard. Maybe we can make it easier with a window.internals.performSyncEdenGC(). One other side comment: The createPromiseRejectionEventPromiseCycle subtest of reference-cycle-leaks.html is still an expected failure. Added two IDB tests for reference cycle in GC world: https://bugs.webkit.org/show_bug.cgi?id=194527 |