sourcegraph / go-webkit2

WebKit API bindings (WebKitGTK+ v2) for Go
https://sourcegraph.com/github.com/sourcegraph/go-webkit2
Other
313 stars 61 forks source link

GAsyncReadyCallback GarbageCollector bug correction and a few other improvments. #11

Closed mdimec4 closed 7 years ago

mdimec4 commented 10 years ago

Hi

I have experienced many crashes connected with GAsyncReadyCallback like this one: http://pastebin.com/ybgWswdU ( Notice 0xdeaddeaddeaddead pointer.) So I have prepared a solution for the problem and I want to share it with you.

If you look at: https://github.com/sourcegraph/go-webkit2/blob/master/webkit2/gasyncreadycallback.go and https://github.com/sourcegraph/go-webkit2/blob/master/webkit2/webview.go you will see the problem:

  1. Callback variable from GetSnapshot or RunJavaScript goes out of Go runtime view. The only reference that remains to it is inside the C world of gtkwebkit2.
  2. Go GC may collect callback function since it is not referenced by a runtime. Callback becomes Dangling pointer
  3. gtkwebkit2 calls _go_gasyncreadycallback_call(), which treys to call callback, which have been removed by GC before. 0xdeaddeaddeaddead pointer and crash
sqs commented 10 years ago

Thanks for doing this work!

1) Is visionect/gotk3 current? So I can get rid of the sqs/gotk3 fork? 2) Can you please fix spelling: s/custum/Custom/i?

mdimec4 commented 10 years ago
  1. visionect/gotk3 haven't been updated since 6-th of April. It also has few of branch special features which haven't been merged back to conformal/gotk yet. But I believe that it
    is still much more current than sqs/gotk3. And has much more features :)
  2. Sorry and thank you for alerting me. I will fix this when I get to work tomorrow.
jrick commented 10 years ago

Sorry for the semi-OT, but were you looking to get those changes merged to upstream gotk3? Are they ready for a PR?

mdimec4 commented 10 years ago

I will have to review them with my College who mostly made them. Then I will push them to repository and create merge request to you.

In the meanwhile you both don't need to wait for me. Those changes are not the one that go-webkit2 would depend on them. I believe that go-webkit2 should work just OK with conformal/gotk3.

My goal is also to merge changes back to the upstream branches.

sqs commented 10 years ago

If go-webkit2 will work with conformal/gotk3, could you also change the import paths to use that repository? Thanks!

mdimec4 commented 10 years ago

No I can't do this. We use visionect/gotk3 in a few applications. For the situations like this when we need to quickly add or fix something, we keep separate branches. We will do this for at least the time, while we are activity developing. I will take a care to contribute back to both sourcegraph/go-webkit2 and conformal/gotk3.

jrick commented 10 years ago

If you are already aware of this I apologize in advance, but depending on your use case it may not even be necessary to fork and change the import paths. Instead, clone upstream and then add another git remote for your Github fork. You can then periodically pull origin to update the master branch from upstream, and push feature branches to github. Keep those feature branches merged with master, and you shouldn't have any merge conflicts when they're ready for a PR. The only issue I see with this is that tools like 'go get' only really work well for git repos when working on the master branch (and I've occasionally seen it switch branches to master when I didn't expect it).