Bug 142958 - unsafety GenericEventQueue operation
Summary: unsafety GenericEventQueue operation
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-03-22 23:11 PDT by Peng Xinchao
Modified: 2015-04-16 19:09 PDT (History)
6 users (show)

See Also:


Attachments
Patch (1.22 KB, patch)
2015-03-22 23:40 PDT, Peng Xinchao
no flags Details | Formatted Diff | Diff
Patch (1.19 KB, patch)
2015-03-22 23:52 PDT, Peng Xinchao
cdumez: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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).