Bug 142958 – unsafety GenericEventQueue operation

• Status: NEW
• Component: New Bugs

[Peng Xinchao]

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.

[Peng Xinchao]

Created attachment 249221 [details]
Patch

[WebKit Commit Bot]

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.

[Peng Xinchao]

Created attachment 249222 [details]
Patch

[Alexey Proskuryakov]

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.

[Chris Dumez]

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.

[Peng Xinchao]

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 .

[Chris Dumez]

(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).