New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dereference remote objects with native code #5293

Merged
merged 4 commits into from Apr 26, 2016

Conversation

Projects
None yet
2 participants
@zcbenz
Contributor

zcbenz commented Apr 26, 2016

Previously we rely on the v8util.setDestructor to dereference the remote objects in JavaScript, however as documented in V8, it is forbidden to call V8 APIs in object's destructor (e.g. the weak callback), and doing so would result in crashs.

This commit removes the JavaScript setDestructor method, and avoids doing the dereference work with V8.

Close #5239.

zcbenz added some commits Apr 26, 2016

Dereference remote objects with native code
Previously we rely on the v8util.setDestructor to dereference the remote
objects in JavaScript, however as documented in V8, it is forbidden to
call V8 APIs in object's destructor (e.g. the weak callback), and doing
so would result in crashs.

This commit removes the JavaScript setDestructor method, and avoids
doing the dereference work with V8.

@zcbenz zcbenz merged commit cdb4444 into master Apr 26, 2016

8 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
electron-linux-arm Build #3124551 succeeded in 82s
Details
electron-linux-ia32 Build #3124552 succeeded in 82s
Details
electron-linux-x64 Build #3124553 succeeded in 106s
Details
electron-mas-x64 Build #956 succeeded in 8 min 13 sec
Details
electron-osx-x64 Build #972 succeeded in 9 min 7 sec
Details
electron-win-ia32 Build #3124554 succeeded in 153s
Details
electron-win-x64 Build #3124555 succeeded in 159s
Details

@zcbenz zcbenz deleted the native-gc branch Apr 26, 2016

@bridiver

This comment has been minimized.

Contributor

bridiver commented on 06cf040 Apr 30, 2016

we saw the crash when updating to chromium 50, but it was crashing because the destructor callback was happening in the FirstWeakCallback. extensions/renderer/gc_callback.cc is basically identical to ObjectLifeMonitor and I fixed the issue by doing the same thing. From gc_callback.cc:

void GCCallback::OnObjectGC(const v8::WeakCallbackInfo<GCCallback>& data) {
  // Usually FirstWeakCallback should do nothing other than reset |object_|
  // and then set a second weak callback to run later. We can sidestep that,
  // because posting a task to the current message loop is all but free - but
  // DO NOT add any more work to this method. The only acceptable place to add
  // code is RunCallback.
  GCCallback* self = data.GetParameter();
  self->object_.Reset();
  base::MessageLoop::current()->PostTask(
      FROM_HERE, base::Bind(&GCCallback::RunCallback,
                            self->weak_ptr_factory_.GetWeakPtr()));
}

I this commit may have overcomplicated things and will also likely still crash because RunDestructor is called in the FirstWeakCallback

This comment has been minimized.

Contributor

zcbenz replied May 1, 2016

It crashed because we were using V8 API in FirstWeakCallback before.

You can find out why we have to do this in FirstWeakCallback at #4733 and #5233.

This comment has been minimized.

Contributor

bridiver replied May 1, 2016

I understand, but the functionality in GCCallback is identical and I fixed the crash in ObjectLifetimeMonitor by duplicating the logic in OnObjectGC. This change adds a lot of code and I'm not sure if it will fix the crash in chromium 50 or not. I'll find out this week when we update to 0.37.8

This comment has been minimized.

Contributor

zcbenz replied May 2, 2016

No it is not identical, GCCallback delays the callback to next tick of message loop which makes a huge difference.

You can find out why this behavior causes troubles from the discussion at #4733 (comment), and it was fixed in #4869. (Sorry I pasted wrong issues in previous comment.)

This comment has been minimized.

Contributor

bridiver replied May 2, 2016

by "identical functionality" I meant that both classes run callbacks on GC. Everything seems to be working fine for us using PostTask in the first callback, but I'll look at #4869 and see if this change works in chromium 50

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment