1162333 - PromiseWorkerProxy should have stronger assertions in debug builds.

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Specifically, GetWorkerPrivate() should assert that mCleanedUpLock is owned by the caller when on the main thread. GetWorkerPromise() should assert it is not on the main thread.

Comment 1

[shubham_sinha]

hi i would like to work on this bug(i am a newbie in this field and have no experience.i know c/c++)

Comment 2

[Mike Hoye [:mhoye]]

Hi, Shubham - That would be great! You can get started here: 

https://developer.mozilla.org/en-US/docs/Introduction 

and once you're set up and ready go go, let us know and Nikhil can tell you a bit about where to start. If you've got any questions about getting set up, email me.

Thanks, and good luck!

Comment 3

[shubham_sinha]

(In reply to Mike Hoye [:mhoye] from comment #2)
> Hi, Shubham - That would be great! You can get started here: 
> 
> https://developer.mozilla.org/en-US/docs/Introduction 
> 
> and once you're set up and ready go go, let us know and Nikhil can tell you
> a bit about where to start. If you've got any questions about getting set
> up, email me.
> 
> Thanks, and good luck!

hey i am ready to go.plz tell me how to get started

Comment 4

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Hi Shubham,

If you see mozilla::Mutex it has methods that check that a certain thread owns it.
What you want to do is, in GetWorkerPrivate()

#ifdef DEBUG
  if (NS_IsMainThread()) {
    // assert that mCleanUpLock is held by the current thread.
  }
#endif

And similarly for GetWorkerPromise()

Comment 5

[shubham_sinha]

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #4)
> Hi Shubham,
> 
> If you see mozilla::Mutex it has methods that check that a certain thread
> owns it.
> What you want to do is, in GetWorkerPrivate()
> 
> #ifdef DEBUG
>   if (NS_IsMainThread()) {
>     // assert that mCleanUpLock is held by the current thread.
>   }
> #endif
> 
> And similarly for GetWorkerPromise()

can you plz provide me with the paths of the files?

Comment 6

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

dom/promise/{Promise.cpp,PromiseWorkerProxy.h}

If you need help from me, please use the "Need more information from" field below, otherwise i end up ignoring them.

Comment 7

[Marco]

If noone is working I'd like to take care of it.

Comment 8

[Mike Hoye [:mhoye]]

Hi, Marco - it looks like Shubham is on top of this, can you email me at [e-mail] so I can set you up with another one?

Comment 9

[Jinank Jain[:jj]]

Attachment: pv1.diff

I just gave a try could someone please review my patch

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Click the [details] link and set 'review' to '?' and put :nsm to the field which shows up.

Comment 11

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8612764 [details] [diff] [review]
pv1.diff

Review of attachment 8612764 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good except for the indentation.

::: dom/promise/Promise.cpp
@@ +1573,5 @@
>  PromiseWorkerProxy::GetWorkerPrivate() const
>  {
>    // It's ok to race on |mCleanedUp|, because it will never cause us to fire
>    // the assertion when we should not.
> +  #ifdef DEBUG

No need to indent preprocessor directives. They always go at the beginning of the line.

@@ +1582,1 @@
>    MOZ_ASSERT(!mCleanedUp);

Move this back up to the relevant comment.

@@ +1586,5 @@
>  
>  Promise*
>  PromiseWorkerProxy::GetWorkerPromise() const
>  {
> +  #ifdef DEBUG

Same here.

Comment 12

[Jinank Jain[:jj]]

Attachment: pv2.diff

I have fixed those things up.

Comment 13

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8612972 [details] [diff] [review]
pv2.diff

Review of attachment 8612972 [details] [diff] [review]:
-----------------------------------------------------------------

As a general rule, once an r+ has been granted, you don't have to ask again after fixing the comments, unless the reviewer has explicitly asked for it.

If you have level 1 access, please push to try server, otherwise I'll do so.

Comment 14

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8612972 [details] [diff] [review]
pv2.diff

Review of attachment 8612972 [details] [diff] [review]:
-----------------------------------------------------------------

Ah after reading my own bug description again, this is actually wrong. I should've been clearer before about GetWorkerPromise().

::: dom/promise/Promise.cpp
@@ +1593,5 @@
> +  if (NS_IsMainThread()) {
> +    mCleanUpLock.AssertCurrentThreadOwns();
> +  }
> +#endif
> +

This assertion should be changed to be

#ifdef DEBUG
  WorkerPrivate* worker = GetCurrentThreadWorkerPrivate();
  MOZ_ASSERT(worker);
  worker->AssertIsOnWorkerThread();
#endif

since we want to assert that the worker promise is only accessed on the worker thread.

Comment 15

[Jinank Jain[:jj]]

Attachment: pv3.diff

Comment 16

[Jinank Jain[:jj]]

Any Updates ?

Comment 17

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5833ba51a7cd

Comment 18

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Jinank, please fix the test failures on try. All of them will likely involve acquiring and checking the cleanup mutex from the PromiseWorkerProxy before touching the WorkerPrivate. The current culprit is ClientFocusRunnable::DispatchResult(), but after you fix that, there may be others. This is the correct way to do it - https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerClients.cpp#108

Comment 19

[Jinank Jain[:jj]]

Attachment: pv4.diff

Comment 20

[Jinank Jain[:jj]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=064f37885249

Comment 21

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8620624 [details] [diff] [review]
pv4.diff

Review of attachment 8620624 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/promise/Promise.cpp
@@ +1580,5 @@
> +#ifdef DEBUG
> +  if (NS_IsMainThread()) {
> +    mCleanUpLock.AssertCurrentThreadOwns();
> +  }
> +#endif

Code that uses this should be modified as I mentioned in a previous comment to make sure it acquires this mutex. ClientFocusRunnable does not seem to do so.

@@ +1591,5 @@
>  {
> +
> +#ifdef DEBUG
> +  MutexAutoLock lock(mPromiseProxy->GetCleanUpLock());
> +  if (mPromiseProxy->IsClean()) {

I think you misunderstood me. This should not be acquiring the mutex, it should only be asserting is on worker thread.

Comment 22

[Jinank Jain[:jj]]

Attachment: pv5.diff

Comment 23

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

The patch still hasn't been updated with the changes to ClientFocusRunnable https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerWindowClient.cpp?from=ClientFocusRunnable&case=true#101 where it needs to acquire the lock.

Also please set the review? flag on the attachment and not needinfo?

Comment 24

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a55356e0e7e

Comment 25

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: MozReview Request: Bug 1162333 - Add stronger assertions to PromiseWorkerProxy in debug builds. r?catalinb

Bug 1162333 - Add stronger assertions to PromiseWorkerProxy in debug builds. r?catalinb

Comment 26

[Cătălin Badea (:catalinb)]

Comment on attachment 8643944 [details]
MozReview Request: Bug 1162333 - Add stronger assertions to PromiseWorkerProxy in debug builds. r?catalinb

https://reviewboard.mozilla.org/r/15185/#review14569

Ship It!

Comment 27

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=85d8854a7f3e

Comment 28

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/5fc9ba7d1273e1e9e6320b502f9753c8515fe2b4
changeset:  5fc9ba7d1273e1e9e6320b502f9753c8515fe2b4
user:       Jinank Jain <[e-mail]>
date:       Thu Jun 11 00:35:18 2015 +0200
description:
Bug 1162333 - Add stronger assertions to PromiseWorkerProxy in debug builds. r=catalinb

Comment 29

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/7bc0c166a74454ae2e589a9a5c886666ea935bd2
changeset:  7bc0c166a74454ae2e589a9a5c886666ea935bd2
user:       Nikhil Marathe <[e-mail]>
date:       Tue Aug 18 10:00:35 2015 -0700
description:
Bug 1162333 - Fix windows bustage. a=bustage CLOSED TREE

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5fc9ba7d1273
https://hg.mozilla.org/mozilla-central/rev/7bc0c166a744