Bug 24152 – Add confirmMessageFromWorkerObject to WorkerObjectProxy.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Jian Li]

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.

[Jian Li]

Created attachment 27958 [details]
Proposed Patch

[David Levin]

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?

[Jian Li]

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?
> 

[David Levin]

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?)

[Jian Li]

Created attachment 28006 [details]
Proposed Patch

[Jian Li]

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?)
> 

[David Levin]

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.

[Alexey Proskuryakov]

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

[Jian Li]

Created attachment 28016 [details]
Proposed Patch

Fix per David's feedback.

[Jian Li]

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

[Jian Li]

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

[David Levin]

Thanks Jian, Looks good to me .

[Alexey Proskuryakov]

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;

[Jian Li]

Created attachment 28074 [details]
Proposed Patch

[Jian Li]

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

[Alexey Proskuryakov]

(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?

[Jian Li]

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

[Jian Li]

Created attachment 28081 [details]
Proposed Patch

Changed to expose askedToTerminate in WorkerContextProxy per discussion with ap.

[Jian Li]

Created attachment 28084 [details]
Proposed Patch

[Jian Li]

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.

[Jian Li]

Need to add virtual void confirmMessageFromWorkerObject(bool hasPendingActivity) to WorkerObjectProxy.

[Jian Li]

Created attachment 28096 [details]
Proposed Patch

[Alexey Proskuryakov]

Comment on attachment 28096 [details]
Proposed Patch

r=me

[David Levin]

Committed as r41305.