Remove domain level lookup from ServiceWorkerManager

RESOLVED FIXED in Firefox 38

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: nsm, Assigned: diorahman, Mentored)

Tracking

33 Branch
mozilla38
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox38 fixed)

Details

(Whiteboard: [good first bug][lang=C++])

Attachments

(1 attachment, 2 obsolete attachments)

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.
Mentor: nsm.nikhil
Whiteboard: [good first bug][lang=C++]

Comment 1

4 years ago
I would like to work on the bug. Can you please explain how should I proceed?
I would also love to help :)
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

4 years ago
Hi, I am new to open source and would like to work on this bug. How should I go about it?
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!
(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.
I would like to take it up, if that is okay :)
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 

mhoye@mozilla.com 

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.
Prabhjyot, how soon can you work on this?
Flags: needinfo?(prabhjyotsingh95)
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)
(Assignee)

Comment 11

4 years ago
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()`.
Flags: needinfo?(nsm.nikhil)
(Assignee)

Comment 12

4 years ago
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.

Thanks
(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.
Flags: needinfo?(prabhjyotsingh95)
Flags: needinfo?(nsm.nikhil)
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.
Attachment #8560989 - Flags: feedback+
(Assignee)

Comment 15

4 years ago
Created attachment 8561197 [details] [diff] [review]
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!
Attachment #8560989 - Attachment is obsolete: true
Attachment #8561197 - Flags: review?(nsm.nikhil)
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)
Attachment #8561197 - Flags: review?(nsm.nikhil) → review+
Assignee: nobody → diorahman
(Assignee)

Comment 17

4 years ago
Created attachment 8561587 [details] [diff] [review]
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!
Attachment #8561197 - Attachment is obsolete: true
Flags: needinfo?(nsm.nikhil)
Attachment #8561587 - Flags: review+
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!
Flags: needinfo?(nsm.nikhil)
https://hg.mozilla.org/mozilla-central/rev/efa2c7310cd1
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.