25009 2009-04-02 11:59:07 -0700 Upstream changes to WorkerContextExecutionProxy for V8 bindings in order to use V8EventListenerList as container. 2009-04-03 15:25:56 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 25004 1 jianli jianli dglazkov oldest_to_newest 116329 0 jianli 2009-04-02 11:59:07 -0700 Upstream changes to WorkerContextExecutionProxy for V8 bindings in order to use V8EventListenerList as container. 116330 1 29203 jianli 2009-04-02 12:04:59 -0700 Created attachment 29203 Proposed Patch Please land this patch after 25004 is landed and brought downstream. 116340 2 29203 fishd 2009-04-02 13:15:53 -0700 Comment on attachment 29203 Proposed Patch > +++ b/WebCore/bindings/v8/WorkerContextExecutionProxy.cpp > @@ -62,6 +62,9 @@ WorkerContextExecutionProxy::WorkerContextExecutionProxy(WorkerContext* workerCo > : m_workerContext(workerContext) > , m_recursion(0) > { > + // Need to use lock since V8EventListenerList constructor creates HandleScope. > + v8::Locker locker; > + m_listeners.set(new V8EventListenerList("m_listeners")); Why not put the v8::Locker closer to where the HandleScope is used? As is, if someone changes V8EventListenerList to no longer use a HandleScope, it won't be obvious that they should also fix this code to no longer use a locker. > + { > + // Need to use lock since V8EventListenerList::clear() creates HandleScope. > + v8::Locker locker; > + m_listeners->clear(); > + } Same issue here. Maybe the locker should move into the clear method? > + // Need to use lock since V8EventListenerList::add() creates HandleScope. > + v8::Locker locker; > + m_listeners->add(newListener.get()); > + } ditto > + // Need to use lock since V8EventListenerList::remove() creates HandleScope. > + v8::Locker locker; > + m_listeners->remove(listener); ditto Or, perhaps the issue is that when V8EventListenerList is used on the main thread of WebKit, we don't want to have it use v8::Locker? If that is the case, then perhaps it would be wise to create a wrapper class for V8EventlistenerList that encapsulates the locker usage? Or maybe that is just plain overkill? 116344 3 jianli 2009-04-02 13:32:13 -0700 (In reply to comment #2) > (From update of attachment 29203 [review]) > > +++ b/WebCore/bindings/v8/WorkerContextExecutionProxy.cpp > > @@ -62,6 +62,9 @@ WorkerContextExecutionProxy::WorkerContextExecutionProxy(WorkerContext* workerCo > > : m_workerContext(workerContext) > > , m_recursion(0) > > { > > + // Need to use lock since V8EventListenerList constructor creates HandleScope. > > + v8::Locker locker; > > + m_listeners.set(new V8EventListenerList("m_listeners")); > > Why not put the v8::Locker closer to where the HandleScope is used? > > As is, if someone changes V8EventListenerList to no longer use a HandleScope, > it > won't be obvious that they should also fix this code to no longer use a locker. > > > > + { > > + // Need to use lock since V8EventListenerList::clear() creates HandleScope. > > + v8::Locker locker; > > + m_listeners->clear(); > > + } > > Same issue here. Maybe the locker should move into the clear method? > > > > + // Need to use lock since V8EventListenerList::add() creates HandleScope. > > + v8::Locker locker; > > + m_listeners->add(newListener.get()); > > + } > > ditto > > > > + // Need to use lock since V8EventListenerList::remove() creates HandleScope. > > + v8::Locker locker; > > + m_listeners->remove(listener); > > ditto > > Or, perhaps the issue is that when V8EventListenerList is used on the main > thread of WebKit, we don't want to have it use v8::Locker? If that is the > case, then perhaps it would be wise to create a wrapper class for > V8EventlistenerList that encapsulates the locker usage? Or maybe that is > just plain overkill? > Yes, V8EventListenerList could be used on the main thread of WebKit that does not use any v8::Locker. I think I can add another constructor to V8EventListenerList that takes additional parameter to indicate if v8::Locker should be used. How do you think? 116346 4 jianli 2009-04-02 14:01:10 -0700 (In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 29203 [review] [review]) > > > +++ b/WebCore/bindings/v8/WorkerContextExecutionProxy.cpp > > > @@ -62,6 +62,9 @@ WorkerContextExecutionProxy::WorkerContextExecutionProxy(WorkerContext* workerCo > > > : m_workerContext(workerContext) > > > , m_recursion(0) > > > { > > > + // Need to use lock since V8EventListenerList constructor creates HandleScope. > > > + v8::Locker locker; > > > + m_listeners.set(new V8EventListenerList("m_listeners")); > > > > Why not put the v8::Locker closer to where the HandleScope is used? > > > > As is, if someone changes V8EventListenerList to no longer use a HandleScope, > > it > > won't be obvious that they should also fix this code to no longer use a locker. > > > > > > > + { > > > + // Need to use lock since V8EventListenerList::clear() creates HandleScope. > > > + v8::Locker locker; > > > + m_listeners->clear(); > > > + } > > > > Same issue here. Maybe the locker should move into the clear method? > > > > > > > + // Need to use lock since V8EventListenerList::add() creates HandleScope. > > > + v8::Locker locker; > > > + m_listeners->add(newListener.get()); > > > + } > > > > ditto > > > > > > > + // Need to use lock since V8EventListenerList::remove() creates HandleScope. > > > + v8::Locker locker; > > > + m_listeners->remove(listener); > > > > ditto > > > > Or, perhaps the issue is that when V8EventListenerList is used on the main > > thread of WebKit, we don't want to have it use v8::Locker? If that is the > > case, then perhaps it would be wise to create a wrapper class for > > V8EventlistenerList that encapsulates the locker usage? Or maybe that is > > just plain overkill? > > > Yes, V8EventListenerList could be used on the main thread of WebKit that does > not use any v8::Locker. I think I can add another constructor to > V8EventListenerList that takes additional parameter to indicate if v8::Locker > should be used. How do you think? > After talking with Darin, we feel that doing subclassing or adding flag in V8EventListenerList is too much pain. So we stay with what it is now. 116497 5 dimich 2009-04-03 15:25:56 -0700 Landed: http://trac.webkit.org/changeset/42215 29203 2009-04-02 12:04:59 -0700 2009-04-02 13:15:53 -0700 Proposed Patch 25009 text/plain 4743 jianli ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg NDZiZjQ3Yi4uYTYzY2YxMiAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvV2Vi Q29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxNyBAQAorMjAwOS0wNC0wMiAgSmlhbiBMaSAgPGpp YW5saUBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISku CisKKyAgICAgICAgVXBzdHJlYW0gY2hhbmdlcyB0byBXb3JrZXJDb250ZXh0RXhlY3V0aW9uUHJv eHkgZm9yIFY4IGJpbmRpbmdzIGluIG9yZGVyIHRvIHVzZSBWOEV2ZW50TGlzdGVuZXJMaXN0IGFz IGNvbnRhaW5lci4KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dp P2lkPTI1MDA5CisKKyAgICAgICAgKiBiaW5kaW5ncy92OC9Xb3JrZXJDb250ZXh0RXhlY3V0aW9u UHJveHkuY3BwOgorICAgICAgICAoV2ViQ29yZTo6V29ya2VyQ29udGV4dEV4ZWN1dGlvblByb3h5 OjpXb3JrZXJDb250ZXh0RXhlY3V0aW9uUHJveHkpOgorICAgICAgICAoV2ViQ29yZTo6V29ya2Vy Q29udGV4dEV4ZWN1dGlvblByb3h5OjpkaXNwb3NlKToKKyAgICAgICAgKFdlYkNvcmU6Oldvcmtl ckNvbnRleHRFeGVjdXRpb25Qcm94eTo6RmluZE9yQ3JlYXRlRXZlbnRMaXN0ZW5lcik6CisgICAg ICAgIChXZWJDb3JlOjpXb3JrZXJDb250ZXh0RXhlY3V0aW9uUHJveHk6OlJlbW92ZUV2ZW50TGlz dGVuZXIpOgorICAgICAgICAqIGJpbmRpbmdzL3Y4L1dvcmtlckNvbnRleHRFeGVjdXRpb25Qcm94 eS5oOgorCiAyMDA5LTA0LTAxICBCZW4gTXVyZG9jaCAgPGJlbm1AZ29vZ2xlLmNvbT4KIAogICAg ICAgICBGaXggQnVnIDE5NzQzOiBSZWxlYXNlIGJ1aWxkIGZhaWxzIG9uIDMyLWJpdCBXaW5kb3dz CmRpZmYgLS1naXQgYS9XZWJDb3JlL2JpbmRpbmdzL3Y4L1dvcmtlckNvbnRleHRFeGVjdXRpb25Q cm94eS5jcHAgYi9XZWJDb3JlL2JpbmRpbmdzL3Y4L1dvcmtlckNvbnRleHRFeGVjdXRpb25Qcm94 eS5jcHAKaW5kZXggYTQ2YWEyNC4uYWIyZTAwYyAxMDA2NDQKLS0tIGEvV2ViQ29yZS9iaW5kaW5n cy92OC9Xb3JrZXJDb250ZXh0RXhlY3V0aW9uUHJveHkuY3BwCisrKyBiL1dlYkNvcmUvYmluZGlu Z3MvdjgvV29ya2VyQ29udGV4dEV4ZWN1dGlvblByb3h5LmNwcApAQCAtNjIsNiArNjIsOSBAQCBX b3JrZXJDb250ZXh0RXhlY3V0aW9uUHJveHk6OldvcmtlckNvbnRleHRFeGVjdXRpb25Qcm94eShX b3JrZXJDb250ZXh0KiB3b3JrZXJDbwogICAgIDogbV93b3JrZXJDb250ZXh0KHdvcmtlckNvbnRl eHQpCiAgICAgLCBtX3JlY3Vyc2lvbigwKQogeworICAgIC8vIE5lZWQgdG8gdXNlIGxvY2sgc2lu Y2UgVjhFdmVudExpc3RlbmVyTGlzdCBjb25zdHJ1Y3RvciBjcmVhdGVzIEhhbmRsZVNjb3BlLgor ICAgIHY4OjpMb2NrZXIgbG9ja2VyOworICAgIG1fbGlzdGVuZXJzLnNldChuZXcgVjhFdmVudExp c3RlbmVyTGlzdCgibV9saXN0ZW5lcnMiKSk7CiB9CiAKIFdvcmtlckNvbnRleHRFeGVjdXRpb25Q cm94eTo6fldvcmtlckNvbnRleHRFeGVjdXRpb25Qcm94eSgpCkBAIC03MiwxMCArNzUsMTQgQEAg V29ya2VyQ29udGV4dEV4ZWN1dGlvblByb3h5Ojp+V29ya2VyQ29udGV4dEV4ZWN1dGlvblByb3h5 KCkKIHZvaWQgV29ya2VyQ29udGV4dEV4ZWN1dGlvblByb3h5OjpkaXNwb3NlKCkKIHsKICAgICAv LyBEaXNjb25uZWN0IGFsbCBldmVudCBsaXN0ZW5lcnMuCi0gICAgZm9yIChzaXplX3QgbGlzdGVu ZXJJbmRleCA9IDA7IGxpc3RlbmVySW5kZXggPCBtX2xpc3RlbmVycy5zaXplKCk7ICsrbGlzdGVu ZXJJbmRleCkKLSAgICAgICBtX2xpc3RlbmVyc1tsaXN0ZW5lckluZGV4XS0+ZGlzY29ubmVjdCgp OworICAgIGZvciAoVjhFdmVudExpc3RlbmVyTGlzdDo6aXRlcmF0b3IgaXRlcmF0b3IobV9saXN0 ZW5lcnMtPmJlZ2luKCkpOyBpdGVyYXRvciAhPSBtX2xpc3RlbmVycy0+ZW5kKCk7ICsraXRlcmF0 b3IpCisgICAgICAgc3RhdGljX2Nhc3Q8VjhXb3JrZXJDb250ZXh0RXZlbnRMaXN0ZW5lcio+KCpp dGVyYXRvciktPmRpc2Nvbm5lY3QoKTsKIAotICAgIG1fbGlzdGVuZXJzLmNsZWFyKCk7CisgICAg eworICAgICAgICAvLyBOZWVkIHRvIHVzZSBsb2NrIHNpbmNlIFY4RXZlbnRMaXN0ZW5lckxpc3Q6 OmNsZWFyKCkgY3JlYXRlcyBIYW5kbGVTY29wZS4KKyAgICAgICAgdjg6OkxvY2tlciBsb2NrZXI7 CisgICAgICAgIG1fbGlzdGVuZXJzLT5jbGVhcigpOworICAgIH0KIAogICAgIC8vIERldGFjaCBh bGwgZXZlbnRzIGZyb20gdGhlaXIgSlMgd3JhcHBlcnMuCiAgICAgZm9yIChzaXplX3QgZXZlbnRJ bmRleCA9IDA7IGV2ZW50SW5kZXggPCBtX2V2ZW50cy5zaXplKCk7ICsrZXZlbnRJbmRleCkgewpA QCAtMzI0LDI5ICszMzEsMjYgQEAgUGFzc1JlZlB0cjxWOEV2ZW50TGlzdGVuZXI+IFdvcmtlckNv bnRleHRFeGVjdXRpb25Qcm94eTo6RmluZE9yQ3JlYXRlRXZlbnRMaXN0ZW4KICAgICBpZiAoIW9i amVjdC0+SXNPYmplY3QoKSkKICAgICAgICAgcmV0dXJuIDA7CiAKLSAgICBmb3IgKHNpemVfdCBp bmRleCA9IDA7IGluZGV4IDwgbV9saXN0ZW5lcnMuc2l6ZSgpOyArK2luZGV4KSB7Ci0gICAgICAg IFY4RXZlbnRMaXN0ZW5lciogZWwgPSBtX2xpc3RlbmVyc1tpbmRleF07Ci0gICAgICAgIGlmIChl bC0+aXNJbmxpbmUoKSA9PSBpc0lubGluZSAmJiBlbC0+Z2V0TGlzdGVuZXJPYmplY3QoKSA9PSBv YmplY3QpCi0gICAgICAgICAgICByZXR1cm4gZWw7Ci0gICAgfQorICAgIFY4RXZlbnRMaXN0ZW5l ciogbGlzdGVuZXIgPSBtX2xpc3RlbmVycy0+ZmluZChvYmplY3QtPlRvT2JqZWN0KCksIGlzSW5s aW5lKTsKICAgICBpZiAoZmluZE9ubHkpCi0gICAgICAgIHJldHVybiBOVUxMOworICAgICAgICBy ZXR1cm4gbGlzdGVuZXI7CiAKICAgICAvLyBDcmVhdGUgYSBuZXcgb25lLCBhbmQgYWRkIHRvIGNh Y2hlLgotICAgIFJlZlB0cjxWOFdvcmtlckNvbnRleHRFdmVudExpc3RlbmVyPiBsaXN0ZW5lciA9 IFY4V29ya2VyQ29udGV4dEV2ZW50TGlzdGVuZXI6OmNyZWF0ZSh0aGlzLCB2ODo6TG9jYWw8djg6 Ok9iamVjdD46OkNhc3Qob2JqZWN0KSwgaXNJbmxpbmUpOwotICAgIG1fbGlzdGVuZXJzLmFwcGVu ZChsaXN0ZW5lci5nZXQoKSk7CisgICAgUmVmUHRyPFY4V29ya2VyQ29udGV4dEV2ZW50TGlzdGVu ZXI+IG5ld0xpc3RlbmVyID0gVjhXb3JrZXJDb250ZXh0RXZlbnRMaXN0ZW5lcjo6Y3JlYXRlKHRo aXMsIHY4OjpMb2NhbDx2ODo6T2JqZWN0Pjo6Q2FzdChvYmplY3QpLCBpc0lubGluZSk7CisgICAg eworICAgICAgICAvLyBOZWVkIHRvIHVzZSBsb2NrIHNpbmNlIFY4RXZlbnRMaXN0ZW5lckxpc3Q6 OmFkZCgpIGNyZWF0ZXMgSGFuZGxlU2NvcGUuCisgICAgICAgIHY4OjpMb2NrZXIgbG9ja2VyOwor ICAgICAgICBtX2xpc3RlbmVycy0+YWRkKG5ld0xpc3RlbmVyLmdldCgpKTsKKyAgICB9CiAKLSAg ICByZXR1cm4gbGlzdGVuZXIucmVsZWFzZSgpOworICAgIHJldHVybiBuZXdMaXN0ZW5lci5yZWxl YXNlKCk7CiB9CiAKIHZvaWQgV29ya2VyQ29udGV4dEV4ZWN1dGlvblByb3h5OjpSZW1vdmVFdmVu dExpc3RlbmVyKFY4RXZlbnRMaXN0ZW5lciogbGlzdGVuZXIpCiB7Ci0gICAgZm9yIChzaXplX3Qg aW5kZXggPSAwOyBpbmRleCA8IG1fbGlzdGVuZXJzLnNpemUoKTsgKytpbmRleCkgewotICAgICAg ICBpZiAobV9saXN0ZW5lcnNbaW5kZXhdID09IGxpc3RlbmVyKSB7Ci0gICAgICAgICAgICBtX2xp c3RlbmVycy5yZW1vdmUoaW5kZXgpOwotICAgICAgICAgICAgcmV0dXJuOwotICAgICAgICB9Ci0g ICAgfQorICAgIC8vIE5lZWQgdG8gdXNlIGxvY2sgc2luY2UgVjhFdmVudExpc3RlbmVyTGlzdDo6 cmVtb3ZlKCkgY3JlYXRlcyBIYW5kbGVTY29wZS4KKyAgICB2ODo6TG9ja2VyIGxvY2tlcjsKKyAg ICBtX2xpc3RlbmVycy0+cmVtb3ZlKGxpc3RlbmVyKTsKIH0KIAogdm9pZCBXb3JrZXJDb250ZXh0 RXhlY3V0aW9uUHJveHk6OnRyYWNrRXZlbnQoRXZlbnQqIGV2ZW50KQpkaWZmIC0tZ2l0IGEvV2Vi Q29yZS9iaW5kaW5ncy92OC9Xb3JrZXJDb250ZXh0RXhlY3V0aW9uUHJveHkuaCBiL1dlYkNvcmUv YmluZGluZ3MvdjgvV29ya2VyQ29udGV4dEV4ZWN1dGlvblByb3h5LmgKaW5kZXggMzAyMzY2Ni4u NzBmOWZjNSAxMDA2NDQKLS0tIGEvV2ViQ29yZS9iaW5kaW5ncy92OC9Xb3JrZXJDb250ZXh0RXhl Y3V0aW9uUHJveHkuaAorKysgYi9XZWJDb3JlL2JpbmRpbmdzL3Y4L1dvcmtlckNvbnRleHRFeGVj dXRpb25Qcm94eS5oCkBAIC0zNSw3ICszNSw5IEBACiAjaWYgRU5BQkxFKFdPUktFUlMpCiAKICNp bmNsdWRlIDx2OC5oPgorI2luY2x1ZGUgIlY4RXZlbnRMaXN0ZW5lckxpc3QuaCIKICNpbmNsdWRl ICJWOEluZGV4LmgiCisjaW5jbHVkZSA8d3RmL093blB0ci5oPgogI2luY2x1ZGUgPHd0Zi9WZWN0 b3IuaD4KIAogbmFtZXNwYWNlIFdlYkNvcmUgewpAQCAtOTUsNyArOTcsNyBAQCBuYW1lc3BhY2Ug V2ViQ29yZSB7CiAgICAgICAgIHY4OjpQZXJzaXN0ZW50PHY4OjpDb250ZXh0PiBtX2NvbnRleHQ7 CiAgICAgICAgIGludCBtX3JlY3Vyc2lvbjsKIAotICAgICAgICBWZWN0b3I8VjhXb3JrZXJDb250 ZXh0RXZlbnRMaXN0ZW5lcio+IG1fbGlzdGVuZXJzOworICAgICAgICBPd25QdHI8VjhFdmVudExp c3RlbmVyTGlzdD4gbV9saXN0ZW5lcnM7CiAgICAgICAgIFZlY3RvcjxFdmVudCo+IG1fZXZlbnRz OwogICAgIH07CiAK