823953 - Improve sync queue handling

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Our sync queue handling is broken when things go wrong.  This cleans it up and fixes a known hang.

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 694859 [details] [diff] [review]
Patch

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

This looks good!

::: dom/workers/WorkerPrivate.h
@@ +850,5 @@
>  
>  JSStructuredCloneCallbacks*
>  ChromeWorkerStructuredCloneCallbacks(bool aMainRuntime);
>  
> +

Nit: Extra line

@@ +874,5 @@
> +  {
> +    return mSyncLoopKey;
> +  }
> +
> +  ~AutoSyncLoopHolder()

Nit: put this after the constructor.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/95807c27bf99

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/95807c27bf99

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 694859 [details] [diff] [review]
Patch

Nominating this because it fixes bug 786987 and will make the sheriffs lives easier.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: Intermittent orange that must be manually starred (TBPL suggestions don't work) and increases load on the sheriffs.  Fixes an issue which can cause web workers to not work properly when lots of worker threads are used.
Testing completed: Baked on m-c for several days.
Risk to taking this patch (and alternatives if risky): Low risk.
String or UUID changes made by this patch: None.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Intermittent orange that must be manually starred (TBPL suggestions don't work) and increases load on the sheriffs.  Fixes an issue which can cause web workers to not work properly when lots of worker threads are used.
Fix Landed on Version: Just central so far.

Comment 5

[bhavana bajaj [:bajaj]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #4)
> Comment on attachment 694859 [details] [diff] [review]
> Patch
> 
> Nominating this because it fixes bug 786987 and will make the sheriffs lives
> easier.
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): N/A
> User impact if declined: Intermittent orange that must be manually starred
> (TBPL suggestions don't work) and increases load on the sheriffs.  Fixes an
> issue which can cause web workers to not work properly when lots of worker
> threads are used.
> Testing completed: Baked on m-c for several days.
> Risk to taking this patch (and alternatives if risky): Low risk.
> String or UUID changes made by this patch: None.
> 
> [Approval Request Comment]
> If this is not a sec:{high,crit} bug, please state case for ESR
> consideration:
> User impact if declined: Intermittent orange that must be manually starred
> (TBPL suggestions don't work) and increases load on the sheriffs.  Fixes an
> issue which can cause web workers to not work properly when lots of worker
> threads are used.
> Fix Landed on Version: Just central so far.

Only approving for aurora/b2g18 considering we closer to release Fx 18 and would like to keep the code base consistent unless its a blocker issue.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fd5dd176e0b2
https://hg.mozilla.org/releases/mozilla-b2g18/rev/f7c95f386622

Comment 7

[bhavana bajaj [:bajaj]]

Comment on attachment 694859 [details] [diff] [review]
Patch

Approving on esr based on comment 4.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Looks like this is going to need some love to land on esr17.

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/releases/mozilla-esr17/rev/4c8a09b56b89

Comment 10

[u279076]

Kyle, is there any testing QA needs to do around this issue?

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

No.

Comment 12

[u279076]

Thanks Kyle. Tagging this bug [qa-]. Please add qawanted keyword if you change your mind.