142958 2015-03-22 23:11:40 -0700 unsafety GenericEventQueue operation 2015-04-16 19:09:31 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified NEW P2 Normal --- 1 xinchao.peng webkit-unassigned ap cdumez commit-queue esprehn+autocc jer.noble kangil.han oldest_to_newest 1079223 0 xinchao.peng 2015-03-22 23:11:40 -0700 In GenericEventQueue class, enqueueEvent function to append event to pendingQueues() and sharedTimerFired function to fire and remove event . In fact ， the better method is that the close function is called in destruction function ,to remove event . It is more safety than the closed is called by external class. 1079227 1 249221 xinchao.peng 2015-03-22 23:40:20 -0700 Created attachment 249221 Patch 1079228 2 commit-queue 2015-03-22 23:42:56 -0700 Attachment 249221 did not pass style-queue: ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style. 1079230 3 249222 xinchao.peng 2015-03-22 23:52:29 -0700 Created attachment 249222 Patch 1079235 4 249222 ap 2015-03-23 00:16:43 -0700 Comment on attachment 249222 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249222&action=review > Source/WebCore/dom/GenericEventQueue.cpp:47 > + if (!m_isClosed) > + close(); What bug does this fix? As far as I can tell, close() doesn't have any effects that would be observable after object destruction, so this proposed change doesn't fix anything. 1086077 5 249222 cdumez 2015-04-16 14:43:21 -0700 Comment on attachment 249222 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249222&action=review > Source/WebCore/ChangeLog:8 > + This changelog does not explain what you are trying to fix. Also you have no layout test to confirm you are fixing something. (I also read the comment on the bug but it is not clear at all) >> Source/WebCore/dom/GenericEventQueue.cpp:47 >> + close(); > > What bug does this fix? > > As far as I can tell, close() doesn't have any effects that would be observable after object destruction, so this proposed change doesn't fix anything. I agree with ap here, calling close() in the destructor is not helpful. All it does is: m_weakPtrFactory.revokeAll(); m_pendingEvents.clear(); Both are already happening during the regular destruction process when those 2 data members are destroyed. 1086219 6 xinchao.peng 2015-04-16 18:56:36 -0700 Let us to analysis Class SourceBuffer . m_asyncEventQueue is a member of SourceBuffer . When SourceBuffer is constructed , m_asyncEventQueue is created . When SourceBuffer::scheduleEvent is called , event is appened into m_pendingEvents and m_weakPtrFactory is appended into GenericEventQueue::pendingQueues(). When SourceBuffer is destructor, GenericEventQueue is also destructor. But if event may be not fired , i think the event should be Invaild . I think these evnet should be deleted . 1086221 7 cdumez 2015-04-16 19:09:31 -0700 (In reply to comment #6) > Let us to analysis Class SourceBuffer . > m_asyncEventQueue is a member of SourceBuffer . When SourceBuffer is > constructed , m_asyncEventQueue is created . > When SourceBuffer::scheduleEvent is called , event is appened into > m_pendingEvents and m_weakPtrFactory is appended into > GenericEventQueue::pendingQueues(). > When SourceBuffer is destructor, GenericEventQueue is also destructor. > But if event may be not fired , i think the event should be Invaild . > I think these evnet should be deleted . As you said, the GenericEventQueue is destroyed. When it is destroyed, its "Deque<RefPtr<Event>> m_pendingEvents" member is also destroyed. When the Dequeue is destroyed, all the events in it will be unref'd (and destroyed if no one else is holding a reference to them). 249221 2015-03-22 23:40:20 -0700 2015-03-22 23:52:19 -0700 Patch bug-142958-20150323143943.patch text/plain 1254 xinchao.peng U3VidmVyc2lvbiBSZXZpc2lvbjogMTgxODQ1CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggYWQ3MjlhZjYyNmMxMGVk MzNiZWQ1ODNlNjg0NWE3ZjJlZWRlNGZkNC4uODQzNzc4OWMxNjIzMzQ5NTY1Mzk5NmU1YjQ5NTI1 ZDMxYTQ3Y2U0ZCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE1IEBACisyMDE1LTAzLTIyICBYaW5j aGFvIFBlbmcgIDx4aW5jaGFvLnBlbmdAc2Ftc3VuZy5jb20+CisKKyAgICAgICAgdW5zYWZldHkg R2VuZXJpY0V2ZW50UXVldWUgb3BlcmF0aW9uCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQu b3JnL3Nob3dfYnVnLmNnaT9pZD0xNDI5NTgKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkg KE9PUFMhKS4KKworICAgICAgICBObyBuZXcgdGVzdHMgKE9PUFMhKS4KKworICAgICAgICAqIGRv bS9HZW5lcmljRXZlbnRRdWV1ZS5jcHA6CisgICAgICAgIChXZWJDb3JlOjpHZW5lcmljRXZlbnRR dWV1ZTo6fkdlbmVyaWNFdmVudFF1ZXVlKToKKwogMjAxNS0wMy0yMiAgQmVuamFtaW4gUG91bGFp biAgPGJlbmphbWluQHdlYmtpdC5vcmc+CiAKICAgICAgICAgQ1NTIFNlbGVjdG9yczogZml4IGF0 dHJpYnV0ZSBjYXNlLWluc2Vuc2l0aXZlIG1hdGNoaW5nIG9mIENvbnRhaW4gYW5kIExpc3QKZGlm ZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL2RvbS9HZW5lcmljRXZlbnRRdWV1ZS5jcHAgYi9Tb3Vy Y2UvV2ViQ29yZS9kb20vR2VuZXJpY0V2ZW50UXVldWUuY3BwCmluZGV4IDEwYzAzNGNjZTdiZGY1 ZGVkOWMzNzJkYTE2NWIwZGZkMjdiMmRkM2QuLjIwY2RkM2IzNTQxZmFiNGIwOTUzZGNmZDY0YjA4 YjkyMzAyMDBiMTYgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL2RvbS9HZW5lcmljRXZlbnRR dWV1ZS5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvZG9tL0dlbmVyaWNFdmVudFF1ZXVlLmNwcApA QCAtNDMsNiArNDMsOCBAQCBHZW5lcmljRXZlbnRRdWV1ZTo6R2VuZXJpY0V2ZW50UXVldWUoRXZl bnRUYXJnZXQmIG93bmVyKQogCiBHZW5lcmljRXZlbnRRdWV1ZTo6fkdlbmVyaWNFdmVudFF1ZXVl KCkKIHsKKyAgICBpZiAoIW1faXNDbG9zZWQpCisgICAgICAgIGNsb3NlKCk7CiB9CiAKIGJvb2wg R2VuZXJpY0V2ZW50UXVldWU6OmVucXVldWVFdmVudChQYXNzUmVmUHRyPEV2ZW50PiBldmVudCkK 249222 2015-03-22 23:52:29 -0700 2015-04-16 14:43:21 -0700 Patch bug-142958-20150323145152.patch text/plain 1223 xinchao.peng U3VidmVyc2lvbiBSZXZpc2lvbjogMTgxODQ1CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggYWQ3MjlhZjYyNmMxMGVk MzNiZWQ1ODNlNjg0NWE3ZjJlZWRlNGZkNC4uODBkNGE4ZjFkMjdjNTUxYjBlMTgzNmFkN2FlYTQw NzkzMmI4Mzc2OCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE0IEBACisyMDE1LTAzLTIyICBYaW5j aGFvIFBlbmcgIDx4aW5jaGFvLnBlbmdAc2Ftc3VuZy5jb20+CisKKyAgICAgICAgdW5zYWZldHkg R2VuZXJpY0V2ZW50UXVldWUgb3BlcmF0aW9uCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQu b3JnL3Nob3dfYnVnLmNnaT9pZD0xNDI5NTgKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkg KE9PUFMhKS4KKworCisgICAgICAgICogZG9tL0dlbmVyaWNFdmVudFF1ZXVlLmNwcDoKKyAgICAg ICAgKFdlYkNvcmU6OkdlbmVyaWNFdmVudFF1ZXVlOjp+R2VuZXJpY0V2ZW50UXVldWUpOgorCiAy MDE1LTAzLTIyICBCZW5qYW1pbiBQb3VsYWluICA8YmVuamFtaW5Ad2Via2l0Lm9yZz4KIAogICAg ICAgICBDU1MgU2VsZWN0b3JzOiBmaXggYXR0cmlidXRlIGNhc2UtaW5zZW5zaXRpdmUgbWF0Y2hp bmcgb2YgQ29udGFpbiBhbmQgTGlzdApkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvZG9tL0dl bmVyaWNFdmVudFF1ZXVlLmNwcCBiL1NvdXJjZS9XZWJDb3JlL2RvbS9HZW5lcmljRXZlbnRRdWV1 ZS5jcHAKaW5kZXggMTBjMDM0Y2NlN2JkZjVkZWQ5YzM3MmRhMTY1YjBkZmQyN2IyZGQzZC4uMjBj ZGQzYjM1NDFmYWI0YjA5NTNkY2ZkNjRiMDhiOTIzMDIwMGIxNiAxMDA2NDQKLS0tIGEvU291cmNl L1dlYkNvcmUvZG9tL0dlbmVyaWNFdmVudFF1ZXVlLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9k b20vR2VuZXJpY0V2ZW50UXVldWUuY3BwCkBAIC00Myw2ICs0Myw4IEBAIEdlbmVyaWNFdmVudFF1 ZXVlOjpHZW5lcmljRXZlbnRRdWV1ZShFdmVudFRhcmdldCYgb3duZXIpCiAKIEdlbmVyaWNFdmVu dFF1ZXVlOjp+R2VuZXJpY0V2ZW50UXVldWUoKQogeworICAgIGlmICghbV9pc0Nsb3NlZCkKKyAg ICAgICAgY2xvc2UoKTsKIH0KIAogYm9vbCBHZW5lcmljRXZlbnRRdWV1ZTo6ZW5xdWV1ZUV2ZW50 KFBhc3NSZWZQdHI8RXZlbnQ+IGV2ZW50KQo=