Bug 27397 – [V8] V8IsolatedWorld keeps a handle to a disposed context

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Matt Perry]

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.

[Adam Barth]

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.

[Adam Barth]

Created attachment 32989 [details]
patch

[Matt Perry]

(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);

[Adam Barth]

(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?

[Adam Barth]

Comment on attachment 32989 [details]
patch

Silly bzt.  Not obsolete.

[Adam Barth]

Created attachment 32991 [details]
Fix weak handle issue

[Matt Perry]

(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 :).

[Mads Ager]

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.

[Adam Barth]

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?

[Mads Ager]

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.

[Adam Barth]

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.

[Adam Barth]

Comment on attachment 32991 [details]
Fix weak handle issue

I believe this is the one we want.

[Eric Seidel (no email)]

Comment on attachment 32991 [details]
Fix weak handle issue

r- for lack of test, or explanation why no test is possible.

[Adam Barth]

Matt, do you have a test case that prompted you to file this bug?

[Matt Perry]

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)

[Adam Barth]

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.

[Matt Perry]

(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.

[Adam Barth]

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.

[Mads Ager]

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.

[Adam Barth]

Comment on attachment 32991 [details]
Fix weak handle issue

The weak callback doesn't currently.  I'll spin up a new version that does.

[Adam Barth]

Created attachment 33164 [details]
patch

[David Levin]

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.

[Adam Barth]

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