This line in V8IsolatedWorld::evaluate: context.Dispose(); should probably be removed. The handle has already been made weak - disposing it makes the handle invalid, causing crashes when it is used later.
Where is |context| made weak? |m_context| is made weak, but isn't that a separate handle? I agree that that line of code can be removed. The intent is to hold a reference to the context for the duration of that function, which we can do without that line.
Created attachment 32989 [details] patch
(In reply to comment #1) > Where is |context| made weak? |m_context| is made weak, but isn't that a > separate handle? My understanding is that m_context and context share the reference to the object. If you want m_context to be a different reference, you need to do: m_context = v8::Persistent<v8::Context>::New(context);
(In reply to comment #3) > My understanding is that m_context and context share the reference to the > object. If you want m_context to be a different reference, you need to do: > m_context = v8::Persistent<v8::Context>::New(context); Ah. I wish the V8 API had better docs. Do you know of any?
Comment on attachment 32989 [details] patch Silly bzt. Not obsolete.
Created attachment 32991 [details] Fix weak handle issue
(In reply to comment #4) > Ah. I wish the V8 API had better docs. Do you know of any? No.. in fact, I tried to search for information to back up my claim, and couldn't find anything definitive. The closest I could come was the comments regarding the "storage cell" on the v8::Persistent constructor and Dispose() methods in the v8 header. So take what I'm saying with a grain of salt :).
Each call to Persistent::New creates a new cell that needs to be disposed by a call to Dispose on a handle that refers to that cell (either this handle or a copy of it created by passing the handle by value). Context::New does a call to Persistent::New under the hood, so anything that you get from a call to Context::New eventually has to be disposed. So, in evaluate, you call createNewContext which returns a newly allocated persistent handle that has to be disposed at some point. Your weak reference callback needs to Dispose the handle on which it is called to actually free the object on the JavaScript side. Once you make that change, I think either of these patches should be fine (but not both): In patch one, you pass on ownership of the allocated persistent to the isolated world which makes the handle weak. Once the handle gets the callback it will be disposed (make sure to dispose the handle in the callback). In that case, it is correct that you should not call Dispose on the handle in the evaluate method since you have passed ownership to the isolated world. In patch two, you keep ownership of the allocated handle and let the isolated world allocate a new handle. In that case, you have to dispose both of the allocated handles when you are done with them. The isolated world handle will be disposed in the weak callback and you need to dispose the other handle as well at the end of evaluate.
We have to manually dispose of persistent handles? I presume we leak the cell if we don't. Hum... We probably have that bug a lot. Why can't the handle destructor dispose of the cell?
Because handles are passed by value and therefore many handles can point to the same cell. We should not have that bug a lot. When objects are removed from the DOMMap tables in the weak handle callbacks, the persistent handle is disposed.
Ok. I'll audit the bindings layer for this issue. We'll also probably want to create some kind of OwnPersistentHandle helper class to avoid leaking the cells. I'm going to cancel these reviews while I figure out the best thing to do in this case. Thanks for explaining how this stuff works.
Comment on attachment 32991 [details] Fix weak handle issue I believe this is the one we want.
Comment on attachment 32991 [details] Fix weak handle issue r- for lack of test, or explanation why no test is possible.
Matt, do you have a test case that prompted you to file this bug?
Created attachment 33123 [details] test extension (In reply to comment #14) > Matt, do you have a test case that prompted you to file this bug? I think the following pieces are key: - Run chrome with extensions enabled, and install/load an extension(*) that has a content script. - Navigate to the page that uses the content script. - Chrome must call into the script later via the chrome extension bindings (or maybe a DOM event listener would work here). * Attached is the extension I used to repro it. Install it with a trunk build of chrome and navigate to google.com. You might need to refresh a few times to repro. (works in --single-process)
Thanks Matt. If we can trigger this with a DOM event, then we can test it using a LayoutTest. From your description, I think that will work. I'll investigate tonight.
(In reply to comment #16) > Thanks Matt. If we can trigger this with a DOM event, then we can test it > using a LayoutTest. From your description, I think that will work. I'll > investigate tonight. Note that I haven't tested it with a DOM event. I was just theorizing that they might share a common code path.
Comment on attachment 32991 [details] Fix weak handle issue Re-nominating this patch because I can't seem to reproduce the issue with a DOM event handler. I'm not sure this is testable without the rest of the extension system.
This looks good to me if the weak reference callback disposes the persistent handle. If the weak reference callback does not dispose the handle there will be a call to Persistent::New with no matching Dispose which will lead to a memory leak.
Comment on attachment 32991 [details] Fix weak handle issue The weak callback doesn't currently. I'll spin up a new version that does.
Created attachment 33164 [details] patch
Comment on attachment 33164 [details] patch Mads Ager says this looks good and he know this a lot better than I do. No style issues. So I'll rubber stamp it based on his review.
Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebCore/ChangeLog M WebCore/bindings/v8/V8IsolatedWorld.cpp Committed r46177 M WebCore/ChangeLog M WebCore/bindings/v8/V8IsolatedWorld.cpp r46177 = ae70d4b8a715c5b701c40e3fc6ed526d23bc32da (trunk) No changes between current HEAD and refs/remotes/trunk Resetting to the latest refs/remotes/trunk http://trac.webkit.org/changeset/46177