Bug 142958

Summary: unsafety GenericEventQueue operation
Product: WebKit Reporter: Peng Xinchao <xinchao.peng>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: NEW ---    
Severity: Normal CC: ap, cdumez, commit-queue, esprehn+autocc, jer.noble, kangil.han
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch cdumez: review-

Description Peng Xinchao 2015-03-22 23:11:40 PDT
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.
Comment 1 Peng Xinchao 2015-03-22 23:40:20 PDT
Created attachment 249221 [details]
Patch
Comment 2 WebKit Commit Bot 2015-03-22 23:42:56 PDT
Attachment 249221 [details] 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.
Comment 3 Peng Xinchao 2015-03-22 23:52:29 PDT
Created attachment 249222 [details]
Patch
Comment 4 Alexey Proskuryakov 2015-03-23 00:16:43 PDT
Comment on attachment 249222 [details]
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.
Comment 5 Chris Dumez 2015-04-16 14:43:21 PDT
Comment on attachment 249222 [details]
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.
Comment 6 Peng Xinchao 2015-04-16 18:56:36 PDT
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 .
Comment 7 Chris Dumez 2015-04-16 19:09:31 PDT
(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).