1253580 - Shared workers need to show the full url in about:debugging#workers

Status: NEW

(Core :: DOM: Workers, defect, P2)

Description

[Soledad Penades [:sole] [:spenades]]

Attachment: Screen Shot 2016-03-04 at 13.03.47.png

Right now I only have two shared workers and it's already complicated to distinguish between them (/js/sharedWorker.js vs worker.js??). I can only imagine it getting worse as more and more sites start using Shared Workers.

(See screenshot)

Comment 1

[Jan Keromnes [:janx]]

dev-doc-needed: When this is fixed, maybe we can write more about Shared Worker debugging in https://developer.mozilla.org/en-US/docs/Tools/about:debugging ?

Comment 2

[Jan Keromnes [:janx]]

Digging a little deeper, we notice that `nsIWorkerDebugger.url` is usually a `scriptSpec` (parsed, absolute URL of the script) e.g. in Service Workers and Dedicated Workers.

For Shared Workers however, `nsIWorkerDebugger.url` looks like the original `scriptURL` argument (unparsed, relative URL of the script) passed to the Shared Worker constructor.

This seems like a bug in our Shared Worker implementation, because the specifications ask that the `scriptURL` argument be parsed[0].

It looks like our `RuntimeService::CreateSharedWorker()` creates its `WorkerPrivate` by passing its own `aScriptURL` argument directly[1] instead of giving it a parsed `scriptSpec` like [2].

Service Workers correctly create their `WorkerPrivate` with a parsed `scriptSpec`[3].

[0] https://html.spec.whatwg.org/multipage/workers.html#sharedworkerglobalscope Shared Worker specification says `scriptURL` should be parsed
[1] https://dxr.mozilla.org/mozilla-central/source/dom/workers/RuntimeService.cpp#2271 Shared Worker instantiates `WorkerPrivate` with own `aScriptURL` argument
[2] https://dxr.mozilla.org/mozilla-central/source/dom/workers/RuntimeService.cpp#2248 A parsed `scriptSpec`
[3] https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerPrivate.cpp#1499 Service Workers used parsed `scriptSpec`

Comment 3

[Jan Keromnes [:janx]]

I blame https://hg.mozilla.org/mozilla-central/rev/9d000928d525

Kyle or Ehsan, do you know who could investigate the problem described in comment 2?

Comment 4

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

Maybe joliu can take a look?

Comment 5

[Jocelyn Liu [:jocelyn] [:joliu]]

I'll investigate this, thanks.

Comment 6

[Jocelyn Liu [:jocelyn] [:joliu]]

A quick update, relative url is shown for both dedicated workers and shared workers, not just shared workers.
Absolute url is shown for service worker.
I'm now looking into WorkerPrivate construction to fix this.

Comment 7

[Jan Keromnes [:janx]]

Thank you Jocelyn for looking into this.

(In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #6)
> A quick update, relative url is shown for both dedicated workers and shared
> workers, not just shared workers.

Actually, as you can see in the attached screenshot of about:debugging ("Other Workers" means "dedicated workers") some dedicated workers have absolute URLs ("resource://...").

Maybe dedicated *web* workers have relative URLs, while dedicated *chrome* workers have absolute URLs?

(The worker from http://html5demos.com/worker does show up as "../js/worker-cruncher.js").

Comment 8

[Jocelyn Liu [:jocelyn] [:joliu]]

(In reply to Jan Keromnes [:janx] from comment #7)
> Thank you Jocelyn for looking into this.
> 
> (In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #6)
> > A quick update, relative url is shown for both dedicated workers and shared
> > workers, not just shared workers.
> 
> Actually, as you can see in the attached screenshot of about:debugging
> ("Other Workers" means "dedicated workers") some dedicated workers have
> absolute URLs ("resource://...").
> 
> Maybe dedicated *web* workers have relative URLs, while dedicated *chrome*
> workers have absolute URLs?
> 
> (The worker from http://html5demos.com/worker does show up as
> "../js/worker-cruncher.js").

Yes, sorry for not being clear enough in my previous comment.
What I meant was dedicated web workers. :)

Comment 9

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

(In reply to Jan Keromnes [:janx] from comment #7)
> Actually, as you can see in the attached screenshot of about:debugging
> ("Other Workers" means "dedicated workers") some dedicated workers have
> absolute URLs ("resource://...").
> 
> Maybe dedicated *web* workers have relative URLs, while dedicated *chrome*
> workers have absolute URLs?
> 
> (The worker from http://html5demos.com/worker does show up as
> "../js/worker-cruncher.js").

ChromeWorkers are dedicated workers ... they probably show up with absolute URLs because they're loaded from absolute URLs.

Comment 10

[Jocelyn Liu [:jocelyn] [:joliu]]

AFAIK, we do have a parsed URL in mLoadInfo.
I have tried two approaches to fix this, and could see absolute URLs in about:debugging.

1) Revise |WorkerDebugger::GetUrl|[1]
   Always return an absolute URL by calling |mWorkerPrivate->GetResolvedScriptURI()->GetSpec(spec)|
2) Revise worker construction at [2]
   pass an absolute URL from aLoadInfo instead of using aScriptURL.
   As a result, mScriptURL now always saves absolute URL instead of the one which was original passed.

In service worker, scriptURL property is included in the standardized ServiceWorker interface.
And the standard clearly states it should be an absolute URL.[3]
But there isn't such property in standardized Worker and SharedWorker interface.
So I wouldn't think it's exactly an implementation bug, but more like what should be the correct behavior when calling |WorkerDebugger::GetUrl|.
That's the reason I first tried approach #1.

At this moment, personally I would prefer approach #2 to make the behavior of |WorkerPrivate::GetScriptURL()| consistent with ServiceWorker.scriptURL.
I'm not that familiar with the entire worker implementation, would need to dig more to make sure not to cause side effect.

The problem I haven't solve here for both #1 and #2 is mResolvedScriptURI of LoadInfo sometimes is null.
Currently I already see I crash the dom/workers/test/test_WorkerDebugger.initialize.xul test because of it, not sure why yet.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/workers/WorkerPrivate.cpp?case=true&from=WorkerPrivate.cpp#3717
[2] https://dxr.mozilla.org/mozilla-central/source/dom/workers/WorkerPrivate.cpp?case=true&from=WorkerPrivate.cpp#4084-4086
[3] https://www.w3.org/TR/service-workers/#service-worker-url

Comment 11

[Jocelyn Liu [:jocelyn] [:joliu]]

So, in SharedWorker case, WorkerPrivate::mLoadInfo::mResolvedScriptURI would not be null.[1]
In dedicated worker case, mResolvedScriptURI is null when we try to create a worker on a worker thread, while it is not null if we're creating a worker on the main thread based on the implementation of |WorkerPrivate::GetLoadInfo|[2].

Kyle, could you kindly answer my questions below to help me shed some light on this?
1) Why we won't assign value to mResolvedScriptURI when we create dedicated workers on a worker thread?
2) Which approach in Comment 10 makes more sense to you? Or none of them make sense?

Please also let me know if I misunderstood anything here, thanks!

[1] https://dxr.mozilla.org/mozilla-central/source/dom/workers/RuntimeService.cpp#2237
[2] https://dxr.mozilla.org/mozilla-central/source/dom/workers/WorkerPrivate.cpp#4136

Comment 12

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

Hmm, I would expect us to set mResolvedScriptURI at http://hg.mozilla.org/mozilla-central/annotate/63be002b4a80/dom/workers/ScriptLoader.cpp#l1092 or http://hg.mozilla.org/mozilla-central/annotate/63be002b4a80/dom/workers/ScriptLoader.cpp#l1195.  Why doesn't that happen?

Comment 13

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

Is mBaseURI already set at http://hg.mozilla.org/mozilla-central/annotate/63be002b4a80/dom/workers/WorkerPrivate.cpp#l3382 or something?

Comment 14

[Jocelyn Liu [:jocelyn] [:joliu]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #12)
> Hmm, I would expect us to set mResolvedScriptURI at
> http://hg.mozilla.org/mozilla-central/annotate/63be002b4a80/dom/workers/
> ScriptLoader.cpp#l1092 or
> http://hg.mozilla.org/mozilla-central/annotate/63be002b4a80/dom/workers/
> ScriptLoader.cpp#l1195.  Why doesn't that happen?

I suppose those codes happens while the runnable at http://hg.mozilla.org/mozilla-central/annotate/63be002b4a80/dom/workers/WorkerPrivate.cpp#l4122 is executed?
Since debugger registration is triggered before this, it's possible that we try to access |WorkerDebugger::GetURL()| while mBaseURI and mResolvedScriptURI are still null, right?

When I run http://mxr.mozilla.org/mozilla-central/source/dom/workers/test/test_WorkerDebugger.initialize.xul test, it creates a child worker and accesses debugger.url in http://mxr.mozilla.org/mozilla-central/source/dom/workers/test/dom_worker_helper.js#66 while mBaseURI and mResolvedScriptURI are null.

Maybe we should return mScriptURL if mResolvedScriptURI is not set yet in GetURL()?
Does this make sense to you?

Comment 15

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: [WIP] Return resolved URL in |WorkerDebugger::GetURL()| if there's an existing resolved URL.

This is a WIP patch simply revise the logic of WorkerDebugger::GetURL().
Several WorkerDebugger tests will need to update also.

Comment 16

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

(In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #14)
> (In reply to Kyle Huey [:khuey] ([e-mail]) from comment #12)
> > Hmm, I would expect us to set mResolvedScriptURI at
> > http://hg.mozilla.org/mozilla-central/annotate/63be002b4a80/dom/workers/
> > ScriptLoader.cpp#l1092 or
> > http://hg.mozilla.org/mozilla-central/annotate/63be002b4a80/dom/workers/
> > ScriptLoader.cpp#l1195.  Why doesn't that happen?
> 
> I suppose those codes happens while the runnable at
> http://hg.mozilla.org/mozilla-central/annotate/63be002b4a80/dom/workers/
> WorkerPrivate.cpp#l4122 is executed?
> Since debugger registration is triggered before this, it's possible that we
> try to access |WorkerDebugger::GetURL()| while mBaseURI and
> mResolvedScriptURI are still null, right?

Yes, that's definitely possible.

> When I run
> http://mxr.mozilla.org/mozilla-central/source/dom/workers/test/
> test_WorkerDebugger.initialize.xul test, it creates a child worker and
> accesses debugger.url in
> http://mxr.mozilla.org/mozilla-central/source/dom/workers/test/
> dom_worker_helper.js#66 while mBaseURI and mResolvedScriptURI are null.
> 
> Maybe we should return mScriptURL if mResolvedScriptURI is not set yet in
> GetURL()?
> Does this make sense to you?

Well, the problem with doing that is that the debugger needs to be notified if mResolvedScriptURI turns out to be different than mScriptURL, right?  ejpbruel, what do you want to do here?  When we fire the debugger callbacks for subworkers we don't have the final URI yet ...

I could imagine .url handing out a promise that resolves when we do get the URI or something.

Comment 17

[Eddy Bruel [:ejpbruel]]

In principle, I'm on board with making the url property hand out a promise that resolves to the the resolved script URI.

We cannot change when the onRegister callback is called. We want the WorkerDebugger for a worker to be
available as soon as possible, because we need it to initialize the debugger, and we want to be able to
initialize the debugger before the worker starts running. To that end, we register the debugger for a worker here: https://dxr.mozilla.org/mozilla-central/source/dom/workers/WorkerPrivate.cpp?case=true&from=WorkerPrivate.cpp#4116, right before the CompileScriptRunnable is dispatched.

Is there a reason why the resolved script URI is not yet available by the time EnableDebugger is called? 

We obtain this value from the mLoadInfo field. As far as I can tell, it is being set here:
https://dxr.mozilla.org/mozilla-central/source/dom/workers/WorkerPrivate.cpp?case=true&from=WorkerPrivate.cpp#1810

StealFrom, in turn, is called in the constructor for WorkerPrivateParent, here:
https://dxr.mozilla.org/mozilla-central/source/dom/workers/WorkerPrivate.cpp?case=true&from=WorkerPrivate.cpp#2255

Since WorkerPrivate inherits from WorkerPrivateParent, that constructor should definitely be called here:
https://dxr.mozilla.org/mozilla-central/source/dom/workers/WorkerPrivate.cpp?case=true&from=WorkerPrivate.cpp#4107

But note that this happens right before the call to EnableDebugger. I'll admit I haven't looked at the code closely, but that seems to imply that mLoadInfo.mResolvedScriptURI should always be defined at that point. Why is it NULL instead?

Comment 18

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

Sorry, perhaps I wasn't clear.  Right now the debugger gets mWorkerPrivate->ScriptURL(), which is always present.  But to fix this bug it needs to get mWorkerPrivate->ResolvedScriptURI(), which is not necessarily available when the registration callback is invoked.  You can test this yourself by changing WorkerDebugger::GetUrl and then running the test mentioned in comment 14.

Comment 19

[Eddy Bruel [:ejpbruel]]

Attachment: Work in progress

Right, so as I understand it, we don't have the full script URL until the main worker script has been loaded, but we don't want to load that script until after the WorkerDebugger is registered (so we get a chance to initialize the debugger server before the worker starts running).

I started working on a prototype to make the url property return a promise instead, as Kyle suggested.

The plan is to make WorkerDebugger::GetURL return a promise, and to resolve that promise when the base URL is set. Ideally, the promise returned by GetURL would always be the same, so we'd only have to resolve that one promise when base URL is set.

The problem I'm currently running into is that Promise::Create requires that we pass it a global object. We can obtain such a global object by passing GetURL an implicit JSContext, and obtaining the current global. However, I believe the nature of WorkerDebugger is such that GetURL can be called from multiple different globals, so we'd end up with multiple different promises, each of which needs to be resolved. That seems needlessly complicated.

A quick search on DXR reveals that we can sometimes obtain the global object for an object if that object has a well defined parent object, but I don't think that is the case for WorkerDebugger either.

Kyle, does WorkerDebugger even have a well-defined global? If so, how can we obtain it? If not, do you think the Promise approach is still viable, or should we try something different, like a method that takes a callback (feels hackish)?

Note that this also affects bug 1241965, because ideally I'd like the onRegister callback to return a promise as well, and I'm running into similar problems with using Promise::All in WorkerDebuggerManager.

Comment 20

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

We discussed an alternative approach on IRC.

Comment 21

[Eddy Bruel [:ejpbruel]]

Attachment: Failed experiment

I got pretty far along, but ran into a fundamental problem with this approach:

1. The debugger should be registered *before* the main worker script is executed. This allows us to initialise a debugger server in the worker before the main worker script starts running. Initialising a debugger server in a worker requires us to be able to exchange messages with the main thread, so debugger runnables should be processed during this time, even though the main worker script hasn’t start running yet.

2. The debugger should be registered *after* the main worker script is loaded. This allows us to obtain the fully resolved script URL for of the main worker script from the WorkerDebugger.

At the moment, we register the debugger right before dispatching CompileScriptRunnable. This registers the debugger before the main worker script is executed, but also before the main worker script is loaded. So this approach meets requirement 1, but not 2.

The approach I tried registers the debugger right before dispatching ScriptExecutorRunnable. At this point, the main worker script has already been loaded, but has not been executed yet. So this approach meets both requirements.

Unfortunately, CompileScriptRunnable enters a sync loop, which we don’t leave until after the script has been executed. We currently don’t process debugger runnables in sync loops, so any debugger runnables we dispatch to the worker won’t actually be run until after the main worker script has executed, defeating the point of registering the debugger before the main worker script is executed in the first place

I’m not really sure how to work around this. I considered processing debugger runnables in sync loops, but I don’t understand the implications of this. Until I get any better ideas, I’m going to leave this bug alone for a bit.

Comment 22

[Eddy Bruel [:ejpbruel]]

Attachment: Proof of concept

Had a conversation with Kyle about this yesterday. He suggested that we could enter a nested debugger event loop from within the sync loop. This allows us to process debugger runnables even when there is a sync loop on the stuck, without any major changes to existing code.

Based on this idea, I created a new proof of concept. It's not quite ready to land, and, but the overall approach looks promising. I haven't fully tested it yet, but I got the basic worker debugging test to pass.

Kyle, before I continue working on this, it seems like a good idea to run what I have so far by you first. Can you take a quick look at this patch today or tomorrow and tell me if you agree with the way I've implemented things? Let me know if you think anything should be done differently.

Comment 23

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

Comment on attachment 8739013 [details] [diff] [review]
Proof of concept

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

This looks generally sane, although I think we need to not start the nested loop/etc if there's no debugger waiting for this thread.

Comment 24

[Eddy Bruel [:ejpbruel]]

Attachment: Initialize the debugger after the main worker script has been loaded.

Okay, I think this patch is ready for review. I addressed your concern of always entering a debugger event loop before we execute the main worker script by only doing so if the debugger has been started by the time all register callbacks have been called.

I also changed the name of the Initialize method on nsIWorkerDebugger to Start, because I felt that initialize was too overloaded with Init/ShutdownDebugger (they are not the same thing: some debuggers may be initialized, but never started).

Comment 25

[Eddy Bruel [:ejpbruel]]

Review ping.

Comment 26

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

Comment on attachment 8740392 [details] [diff] [review]
Initialize the debugger after the main worker script has been loaded.

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

::: dom/workers/ScriptLoader.cpp
@@ +1344,5 @@
> +      // fully initialized at this point.
> +      if (isDebuggerStarted) {
> +        RefPtr<LeaveDebuggerEventLoopRunnable> runnable =
> +          new LeaveDebuggerEventLoopRunnable(mWorkerPrivate);
> +        if (!runnable->Dispatch()) {

This is quasi-racy, which worries me a little.  Specifically, if the debugger initialization does a bunch of setImmediate calls to post to the debugger thread event queue, where LeaveDebuggerEventLoopRunnable gets inserted into the queue is undefined.  Notably though, this isn't racy with respect to calls to the main thread, because we are the main thread!

Since this is intended to be temporary (right?!) I'm ok with this landing as is as long as you're aware that this issue exists and are ok with it.  If you wanted to fix this you'd probably need to dispatch the LeaveDebuggerEventLoopRunnable *from the debugger thread* immediately after executing the debugger's initialization function.

Comment 27

[Jan Keromnes [:janx]]

Raising the priority to P1 because only showing relative URLs for Shared and Dedicated Workers makes about:debugging#workers look confusing and almost unusable in some cases.

Comment 28

[Eddy Bruel [:ejpbruel]]

(In reply to Jan Keromnes [:janx] from comment #27)
> Raising the priority to P1 because only showing relative URLs for Shared and
> Dedicated Workers makes about:debugging#workers look confusing and almost
> unusable in some cases.

I'll ask marcom if I can dedicate some cycles to this during our standup tonight.

Comment 29

[Jan Keromnes [:janx]]

Marco, it is important that we find a way to display full URLs instead of relative URLs for Shared Workers in about:debugging (please see comment 27).

Can Eddy dedicate some cycles to this, as requested in comment 28?

Comment 30

[Marco Mucci [:MarcoM]]

Hello Jan,  Yes, Eddy brought up the issue with this bug in the team standup.  He informed the team he would be directing some of his time this release to it.

Comment 31

[Julian Descottes [:jdescottes]]

Eddy, I wanted to have this as a "top-bug" for aboutdebugging but the status seems unclear.
Your last patch got r+, could you describe what remains to be done here?

Comment 32

[Eddy Bruel [:ejpbruel]]

My memory is a bit rusty, but I think the main reason we didn't land this yet was because I wanted to make sure comment 26 wasn't an issue, and then got sidetracked doing other stuff.

Do you feel confident about trying to complete this yourself? If not, I should be able to spare some cycles to take another look at it.