Bug 24152

Summary: Add confirmMessageFromWorkerObject to WorkerObjectProxy.
Product: WebKit Reporter: Jian Li <jianli>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, dimich, jianli, levin
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Proposed Patch
none
Proposed Patch
none
Proposed Patch
ap: review-
Proposed Patch
none
Proposed Patch
none
Proposed Patch
none
Proposed Patch ap: review+

Description Jian Li 2009-02-24 18:45:51 PST
Move the logic to do the unconfirmed message count and pending activity setting from WorkerMessagingProxy to Worker so that it can be used by multiple platforms.
Comment 1 Jian Li 2009-02-24 21:51:03 PST
Created attachment 27958 [details]
Proposed Patch
Comment 2 David Levin 2009-02-24 22:06:27 PST
Just one question, previously the code looked like this:
     return m_contextProxy->hasPendingActivity() || ActiveDOMObject::hasPendingActivity();

where

     m_contextProxy->hasPendingActivity() was "(m_unconfirmedMessageCount || m_workerThreadHadPendingActivity) && !m_askedToTerminate"

but now it is
 
     return m_unconfirmedMessageCount || m_workerContextHadPendingActivity || ActiveDOMObject::hasPendingActivity();

Why is the loss of the m_askedToTerminate condition ok?
Comment 3 Jian Li 2009-02-25 08:07:53 PST
Previously, worker object inquiries WorkerMessagingProxy about these info. So it needs to check if WorkerMessagingProxy is in terminating state or not. Now WorkerMessagingProxy passes these info directly to the worker object. So this check is performed before calling into the worker object (see WorkerMessagingProxy::reportPendingActivityInternal).

(In reply to comment #2)
> Just one question, previously the code looked like this:
>      return m_contextProxy->hasPendingActivity() ||
> ActiveDOMObject::hasPendingActivity();
> 
> where
> 
>      m_contextProxy->hasPendingActivity() was "(m_unconfirmedMessageCount ||
> m_workerThreadHadPendingActivity) && !m_askedToTerminate"
> 
> but now it is
> 
>      return m_unconfirmedMessageCount || m_workerContextHadPendingActivity ||
> ActiveDOMObject::hasPendingActivity();
> 
> Why is the loss of the m_askedToTerminate condition ok?
> 

Comment 4 David Levin 2009-02-25 09:27:05 PST
I don't under this:
  Worker::hasPendingActivity() will return true even after the worker context has been terminated. Previously, it would return false after this.

Why is this change ok?  (Won't this cause GC issues?)
Comment 5 Jian Li 2009-02-25 23:19:41 PST
Created attachment 28006 [details]
Proposed Patch
Comment 6 Jian Li 2009-02-25 23:25:42 PST
Fixed this issue by adding the check in Worker class.

I also fixed the occasional crash I talked to you this afternoon. The fix is to change line "reportPendingActivity(true);" in WorkerMessagingProxy::workerThreadCreated to "m_workerObject->reportPendingActivity(false, true);". The reason for the failure is that workerThreadCreated is executed in main thread and if we call reportPendingActivity(true) it will post a task to the main thread and unfortunately this task might get a chance to be performed at very late time when the proxy goes away.

(In reply to comment #4)
> I don't under this:
>   Worker::hasPendingActivity() will return true even after the worker context
> has been terminated. Previously, it would return false after this.
> 
> Why is this change ok?  (Won't this cause GC issues?)
> 

Comment 7 David Levin 2009-02-25 23:37:53 PST
Worker::hasPendingActivity() has changed behavior in this patch.  I think you want:

   return (!m_askedToTerminate && (m_unconfirmedMessageCount || m_workerContextHadPendingActivity)) || ActiveDOMObject::hasPendingActivity();

and it would be nice to put this part "!m_askedToTerminate && (m_unconfirmedMessageCount || m_workerContextHadPendingActivity" in a local bool to make it more readable. 



> for (unsigned i = 0; i < m_queuedEarlyTasks.size(); ++i)
It would be nice to put m_queuedEarlyTasks.size() in a local variable.

Comment 8 Alexey Proskuryakov 2009-02-26 03:37:48 PST
(In reply to comment #6)
> I also fixed the occasional crash I talked to you this afternoon.

Could you make an automated test case for it? Even if it won't be 100% reliable, it's better than having no test at all.
 
Comment 9 Jian Li 2009-02-26 08:30:28 PST
Created attachment 28016 [details]
Proposed Patch

Fix per David's feedback.
Comment 10 Jian Li 2009-02-26 08:36:30 PST
I am not sure how to add test case for this since it is extremely reproduce.

I encountered this crash when I ran all worker layout tests, like "run-webkit-tests LayoutTest/fast/workers/". This could reproduce occasionally, say 2%. It does not reproduce when I debug it in xcode.

I found out that if I run the test command "run-webkit-tests LayoutTest/fast/workers/" from two terminal windows in parallel, it will add up the repro chance.

If you know how to get a test case for this, please let me know.

(In reply to comment #8)
> (In reply to comment #6)
> > I also fixed the occasional crash I talked to you this afternoon.
> 
> Could you make an automated test case for it? Even if it won't be 100%
> reliable, it's better than having no test at all.
> 
> 

Comment 11 Jian Li 2009-02-26 08:37:39 PST
(In reply to comment #10)
> I am not sure how to add test case for this since it is extremely reproduce.
Sorry, I mean extremely hard-to-reproduce.
> 
> I encountered this crash when I ran all worker layout tests, like
> "run-webkit-tests LayoutTest/fast/workers/". This could reproduce occasionally,
> say 2%. It does not reproduce when I debug it in xcode.
> 
> I found out that if I run the test command "run-webkit-tests
> LayoutTest/fast/workers/" from two terminal windows in parallel, it will add up
> the repro chance.
> 
> If you know how to get a test case for this, please let me know.
> 
> (In reply to comment #8)
> > (In reply to comment #6)
> > > I also fixed the occasional crash I talked to you this afternoon.
> > 
> > Could you make an automated test case for it? Even if it won't be 100%
> > reliable, it's better than having no test at all.
> > 
> > 
> 

Comment 12 David Levin 2009-02-26 11:08:46 PST
Thanks Jian, Looks good to me .
Comment 13 Alexey Proskuryakov 2009-02-27 01:24:49 PST
Comment on attachment 28016 [details]
Proposed Patch

+    bool hasPendingActivity = !m_askedToTerminate && (m_unconfirmedMessageCount || m_workerContextHadPendingActivity);
+    return hasPendingActivity || ActiveDOMObject::hasPendingActivity();

This variable name is not helpful. I suggest workerContextHasPendingActivity.

+        bool m_askedToTerminate;

Why duplicate this member variable in Worker when it's already available in the proxy?

-        m_unconfirmedMessageCount = taskCount;
-        m_workerThreadHadPendingActivity = true; // Worker initialization means a pending activity.
+        m_workerObject->reportPendingActivity(false, true); // Worker initialization means a pending activity.

This patch makes workers that failed to load leak. Previously, m_unconfirmedMessageCount was only set to non-zero after creating the thread, so messages posted to a worker that never spawned a thread didn't count as activity. Consider the following test:

var worker = new Worker("does-not-exist.js");
worker.postMessage(...);
worker = null;
Comment 14 Jian Li 2009-02-27 09:06:13 PST
Created attachment 28074 [details]
Proposed Patch
Comment 15 Jian Li 2009-02-27 09:07:52 PST
(In reply to comment #13)
> (From update of attachment 28016 [details] [review])
> +    bool hasPendingActivity = !m_askedToTerminate &&
> (m_unconfirmedMessageCount || m_workerContextHadPendingActivity);
> +    return hasPendingActivity || ActiveDOMObject::hasPendingActivity();
> 
> This variable name is not helpful. I suggest workerContextHasPendingActivity.

Changed.
> 
> +        bool m_askedToTerminate;
> 
> Why duplicate this member variable in Worker when it's already available in the
> proxy?

Yes, it is already available in the proxy. However it is not exposed in WorkerContextProxy which is the interface Worker knows about. So I have to duplicate the logic.
> 
> -        m_unconfirmedMessageCount = taskCount;
> -        m_workerThreadHadPendingActivity = true; // Worker initialization
> means a pending activity.
> +        m_workerObject->reportPendingActivity(false, true); // Worker
> initialization means a pending activity.
> 
> This patch makes workers that failed to load leak. Previously,
> m_unconfirmedMessageCount was only set to non-zero after creating the thread,
> so messages posted to a worker that never spawned a thread didn't count as
> activity. Consider the following test:
> 
> var worker = new Worker("does-not-exist.js");
> worker.postMessage(...);
> worker = null;
> 
Good catch. I added the check for this.
Comment 16 Alexey Proskuryakov 2009-02-27 09:41:33 PST
(In reply to comment #15)
> Yes, it is already available in the proxy. However it is not exposed in
> WorkerContextProxy which is the interface Worker knows about. So I have to
> duplicate the logic.

Why not expose it then?
Comment 17 Jian Li 2009-02-27 10:05:10 PST
(In reply to comment #16)
> (In reply to comment #15)
> > Yes, it is already available in the proxy. However it is not exposed in
> > WorkerContextProxy which is the interface Worker knows about. So I have to
> > duplicate the logic.
> 
> Why not expose it then?
> 

We could expose it. However, I think this seems not to be part of communication between two proxy interfaces. When worker object calls WorkerContextProxy::terminateWorkerContext, it already knows that it requests to terminate the context. So it just seems not very natural for the worker object to ask the corresponding proxy for the information it knows about.
Comment 18 Jian Li 2009-02-27 10:40:58 PST
Created attachment 28081 [details]
Proposed Patch

Changed to expose askedToTerminate in WorkerContextProxy per discussion with ap.
Comment 19 Jian Li 2009-02-27 10:58:23 PST
Created attachment 28084 [details]
Proposed Patch
Comment 20 Jian Li 2009-02-27 11:48:21 PST
Comment on attachment 28084 [details]
Proposed Patch

Retreat this patch since we're taking a different way to address the issue: exposing one more method in WorkerObjectProxy.
Comment 21 Jian Li 2009-02-27 12:07:12 PST
Need to add virtual void confirmMessageFromWorkerObject(bool hasPendingActivity) to WorkerObjectProxy.
Comment 22 Jian Li 2009-02-27 12:09:21 PST
Created attachment 28096 [details]
Proposed Patch
Comment 23 Alexey Proskuryakov 2009-02-27 12:12:06 PST
Comment on attachment 28096 [details]
Proposed Patch

r=me
Comment 24 David Levin 2009-02-27 15:30:55 PST
Committed as r41305.