1113003 - Remove domain level lookup from ServiceWorkerManager

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

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

Not a blocker, but something to simplify the code. We currently store SW registrations in memory in a 2 level { { origin: [list of SWs] } } table. Since we are always using absolute URLs throughout the implementation, we can get rid of the origin level.

Comment 1

[Nihar Mehta]

I would like to work on the bug. Can you please explain how should I proceed?

Comment 2

[Prabhjyot Sodhi [:psd]]

I would also love to help :)

Comment 3

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

I'd highly recommend waiting until Bug 1113555 is fixed, since that is going to make several destructive changes. That said, dom/workers/ServiceWorkerManager.h - ServiceWorkerManager stores a mDomainMap. We want to get rid of it and hoist the 6 arrays/hashtables it holds into ServiceWorkerManager, along with the utility functions. Then delete the ServiceWorkerDomainInfo struct and get rid of the various indirections in the implementation (get it to build and have all tests in dom/workers/test/serviceworkers/ pass, especially test_scopes). I'll let you figure out the rest yourself, but please needinfo me (the field right below the comment field on bugzilla) if you need help. I do not pay attention to bugs I'm not needinfo'd on.

Comment 4

[shreyas]

Hi, I am new to open source and would like to work on this bug. How should I go about it?

Comment 5

[Mike Hoye [:mhoye]]

Hi, all -

There's a lot of interest in this bug - if one of you could pick it up, I'll make sure we find good starter bugs for everyone. Thanks!

Comment 6

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

(In reply to shreyas from comment #4)
> Hi, I am new to open source and would like to work on this bug. How should I
> go about it?

First, set up your machine to download the Firefox source code and build firefox from source. Then you can make changes to the code as outlined in comment 3.

https://developer.mozilla.org/en-US/docs/Introduction and https://developer.mozilla.org/en-US/docs/Simple_Firefox_build will help you get started.

Comment 7

[Prabhjyot Sodhi [:psd]]

I would like to take it up, if that is okay :)

Comment 8

[Mike Hoye [:mhoye]]

OK, it looks like Prabhjyot will be taking this one up. Shreyas and Nihar, thanks for your interest; could both of you contact me at 

[e-mail] 

and tell me a sentence or two about your skills and interests? I will match you up with bugs that seem like a good fit.

Comment 9

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

Prabhjyot, how soon can you work on this?

Comment 10

[Prabhjyot Sodhi [:psd]]

Hey Nikhil!
A couple of days back, in an irc chat, someone wanted to tackle this bug So I asked him to go ahead with it.
I can work on it tomorrow if there is no activity.
I am not able to locate him through his irc nick (diorahman)

Comment 11

[Dhi Aurrahman]

Hi Nikhil,

I'm Dhi (diorahman). I asked jdm about a good first bug, and jdm asked psd if I could possibly work on this bug instead of psd. That's the story. I hope it will be fine :)

Anyway, I made it compile. Now I'm trying to figure out when the first time the `mServiceWorkerRegistrations` is filled, since for now, I couldn't pass the assertion (`MOZ_ASSERT(mServiceWorkerRegistrations.Contains(registration)`);) inside the `ServiceWorkerManager::AddRegistrationEventListener()`.

Comment 12

[Dhi Aurrahman]

Attachment: First attempt

Hi Nikhil,

This is my first attempt. 

It can pass the `dom/workers/test/serviceworkers/test_scopes.html`.

However it fails on another test, i.e:

11 INFO TEST-UNEXPECTED-FAIL | dom/workers/test/serviceworkers/test_installation_simple.html | First registration should fail with AbortError - expected PASS

I'm still trying to figure out which related line is throwing the AbortError.

Thanks

Comment 13

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

(In reply to Dhi Aurrahman from comment #12)
> Created attachment 8560989 [details] [diff] [review]
> First attempt
> 
> Hi Nikhil,
> 
> This is my first attempt. 
> 
> It can pass the `dom/workers/test/serviceworkers/test_scopes.html`.
> 
> However it fails on another test, i.e:
> 
> 11 INFO TEST-UNEXPECTED-FAIL |
> dom/workers/test/serviceworkers/test_installation_simple.html | First
> registration should fail with AbortError - expected PASS
> 
> I'm still trying to figure out which related line is throwing the AbortError.

I'd  be surprised if this was your fault, there are problems with the SW tests and I have a local patch that fixes some of them. I'll look at your patch.

Comment 14

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

Comment on attachment 8560989 [details] [diff] [review]
First attempt

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

This looks really good! Once you've done the changes mentioned below, please upload the new patch and set the attachment's review? flag to me.
If you have Level 1 access to hg.mozilla.org (I encourage you to apply if you don't), please also post a link to a try build indicating this patch compiles successfully on all platforms in both optimized and debug builds

https://www.mozilla.org/en-US/about/governance/policies/commit/access-policy/

Thanks!

::: dom/workers/ServiceWorkerManager.cpp
@@ +431,5 @@
>            Done(NS_OK);
>            return;
>          }
>        } else {
> +        mRegistration = swm->CreateNewRegistration(mScope); 

Nit: Trailing space.

@@ +436,4 @@
>        }
>  
>        mRegistration->mScriptSpec = mScriptSpec;
> +

Nit: Excess newline.

@@ +865,2 @@
>    MOZ_ASSERT(queue);
> +  

Nit: Trailing spaces.

@@ +871,5 @@
>      new ServiceWorkerRegisterJob(queue, cleanedScope, spec, cb);
>    queue->Append(job);
>  
>    promise.forget(aPromise);
> +

Nit: extra newline.

@@ +1417,5 @@
>    {
>      AssertIsOnMainThread();
>  
>      nsRefPtr<ServiceWorkerManager> swm = ServiceWorkerManager::GetInstance();
> +    

Nit: Trailing spaces.

@@ +1512,5 @@
>                                    uint32_t aColumnNumber,
>                                    uint32_t aFlags)
>  {
>    AssertIsOnMainThread();
> +  

Nit: Trailing space.

@@ +1638,5 @@
>    nsresult rv = aURI->GetSpec(spec);
>    if (NS_WARN_IF(NS_FAILED(rv))) {
>      return nullptr;
>    }
> +  

Nit: Trailing spaces.

::: dom/workers/ServiceWorkerManager.h
@@ +302,5 @@
> +  // wrong this should be replaced with a better structure to avoid the
> +  // memmoves associated with inserting stuff in the middle of the array.
> +  nsTArray<nsCString> mOrderedScopes;
> +
> +  // Scope to registration.

Nit: Please update this comment to say the scope should be a fully qualified valid URL.

@@ +309,5 @@
> +  nsTObserverArray<ServiceWorkerRegistration*> mServiceWorkerRegistrations;
> +
> +  nsRefPtrHashtable<nsISupportsHashKey, ServiceWorkerRegistrationInfo> mControlledDocuments;
> +
> +  nsClassHashtable<nsCStringHashKey, ServiceWorkerJobQueue> mJobQueues;

Nit: Please add a comment for this stating "Maps scopes to job queues."

@@ +324,4 @@
>  
> +  ServiceWorkerRegistrationInfo*
> +  CreateNewRegistration(const nsCString& aScope)
> +  {

Please add a debug-only block of code here (Using  #ifdef DEBUG... #endif) that uses |NS_NewURI(getter_AddRefs(aURI), aScope, nullptr, nullptr)| to ensure that aScope is a valid fully qualified URL. It would be nicer to move this function definition to the .cpp file.

It would go something like:

#ifdef DEBUG
AssertIsOnMainThread();
nsCOMPtr<nsIURI> scopeURI;
nsresult rv = NS_NewURI(getter_AddRefs(scopeURI), aScope, nullptr, nullptr);
MOZ_ASSERT(NS_SUCCEEDED(rv));
#endif

@@ +325,5 @@
> +  ServiceWorkerRegistrationInfo*
> +  CreateNewRegistration(const nsCString& aScope)
> +  {
> +    ServiceWorkerRegistrationInfo* registration =
> +      new ServiceWorkerRegistrationInfo(aScope);

Nit: Can this be moved to the same line while still staying under 80chars?

@@ +329,5 @@
> +      new ServiceWorkerRegistrationInfo(aScope);
> +    // From now on ownership of registration is with
> +    // mServiceWorkerRegistrationInfos.
> +    mServiceWorkerRegistrationInfos.Put(aScope, registration);
> +    ServiceWorkerManager::AddScope(mOrderedScopes, aScope);

This can be just AddScope() now.

Comment 15

[Dhi Aurrahman]

Attachment: Update after comment 1

Hi Nikhil,

The patch is updated based on comment 1. I haven't got the commit access level 1. I have just requested it (https://bugzilla.mozilla.org/show_bug.cgi?id=1130911).

Thanks!

Comment 16

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

Comment on attachment 8561197 [details] [diff] [review]
Update after comment 1

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

This looks good. I'm happy to land it for you (along with making the changes I pointed out) once a try build succeeds.

::: dom/workers/ServiceWorkerManager.h
@@ +302,5 @@
> +  // wrong this should be replaced with a better structure to avoid the
> +  // memmoves associated with inserting stuff in the middle of the array.
> +  nsTArray<nsCString> mOrderedScopes;
> +
> +  // Scope to registration. 

Nit: Trailing space.

@@ +303,5 @@
> +  // memmoves associated with inserting stuff in the middle of the array.
> +  nsTArray<nsCString> mOrderedScopes;
> +
> +  // Scope to registration. 
> +  // The scope should be a fully qualified valid URL.

Please change should to MUST (all caps)

Comment 17

[Dhi Aurrahman]

Attachment: Update after comment 16

Hi Nikhil,

I hope I can carry the r+ from previous round. 

Regarding the commit access: https://bugzilla.mozilla.org/show_bug.cgi?id=1130911#c1

Thanks!

Comment 18

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

Based on the try build here - https://treeherder.mozilla.org/#/jobs?repo=try&revision=df7e94cfab4b - I'm landing this on your behalf. I had to make a few small edits (namely the condition in HandleError() had been reversed). Thanks for the patch!

Comment 19

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/efa2c7310cd1

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/efa2c7310cd1