Summary: | unsafety GenericEventQueue operation | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Peng Xinchao <xinchao.peng> | ||||||
Component: | New Bugs | Assignee: | 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
Peng Xinchao
2015-03-22 23:11:40 PDT
Created attachment 249221 [details]
Patch
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.
Created attachment 249222 [details]
Patch
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 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. 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 . (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). |