Wait until thread exits in CrossThreadTaskHandler::kill()

• Component: New Bugs
• Status: NEW ---

[Sihui Liu]

...

[Sihui Liu]

Created attachment 425954 [details]
Patch

[Chris Dumez]

Comment on attachment 425954 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=425954&action=review

> Source/WTF/ChangeLog:7
> +

Please explain WHY you are making this change.

[Sihui Liu]

Created attachment 425997 [details]
Patch

[Sihui Liu]

(In reply to Chris Dumez from comment #2)
> Comment on attachment 425954 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=425954&action=review
> 
> > Source/WTF/ChangeLog:7
> > +
> 
> Please explain WHY you are making this change.

Added.

[Chris Dumez]

Comment on attachment 425997 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=425997&action=review

> Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.cpp:425
> +    CrossThreadTaskHandler::kill();

I have several concerns about this patch:
1. The verb "kill", to me, is not really associated with "wait for remaining tasks to be processed and wait for thread to exit"
2. The new code hangs the NetworkProcess's main thread on a background thread that is used for database activity (such activity likely includes going to disk). I don't think it is ever OK to hang the main thread on such operations.

I am all for making the code safe but making the code safe by hanging the main thread does not seem like a good trade-off.

[Sihui Liu]

(In reply to Chris Dumez from comment #5)
> Comment on attachment 425997 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=425997&action=review
> 
> > Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.cpp:425
> > +    CrossThreadTaskHandler::kill();
> 
> I have several concerns about this patch:
> 1. The verb "kill", to me, is not really associated with "wait for remaining
> tasks to be processed and wait for thread to exit"

I think so too; can rename it to something like waitUntilFinished().

> 2. The new code hangs the NetworkProcess's main thread on a background
> thread that is used for database activity (such activity likely includes
> going to disk). I don't think it is ever OK to hang the main thread on such
> operations.
> 

ah, I don't think this is a big concern for database thread because WebIDBServer (the only user of CrossThreadTaskHandler for now) is removed in 2 cases:
1. it's idle, no pending tasks or IDB connection from web process. This means the kill operation should be very quick.
2. network process is about to be destroyed (NetworkProcess::didClose). I guess network process does not matter so much at this point.

> I am all for making the code safe but making the code safe by hanging the
> main thread does not seem like a good trade-off.

[Sihui Liu]

Created attachment 426017 [details]
Patch

[Chris Dumez]

Comment on attachment 426017 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=426017&action=review

> Source/WTF/ChangeLog:9
> +        1. CrossThreadQueue<DataType>::waitForMessage() expects a new task before checking m_killed

CrossThreadQueue<DataType>::waitForMessage() should probably be fixed so that it checked m_killed when woken up:
- while (found == m_queue.end()) {
+ while (!m_killed && found == m_queue.end()) {

[Radar WebKit Bug Importer]

<rdar://problem/76938794>