Kyle requested that I opened a new bug for the remainder of the work for bug 757133, so here goes.

Uploaded the wrong patch.

Review ping

Second review ping.

Comment on attachment 8514902 [details] [diff] [review] Implement WorkerDebugger.postMessage New patches are coming.

This one might be a bit tricky. The key thing to understand is that I use sync loops to implement nested event loops, and the debugger API knows to post its events to the topmost sync loop, if any.

Normal timeouts are not aware of nested event loops, so the debugger needs its own implementation. I've tried to keep debugger timeouts as separate as possible from normal timeouts to make sure I don't subtly change the behavior of the latter.

The debugger API makes heavy use of CommonJS modules, and that means we need a sandbox API. I've mostly copied over the one in xpconnect, though I've kept it much simpler.

We need to distinguish between suspending a worker (i.e. suspending its event queue, but not blocking the worker from running) and freezing a worker (i.e. blocking the worker from running). What we call suspending now is more accurately called freezing, so we have to rename some things.

This patch makes it possible to suspend a workers event queue without blocking the worker from running. This is necessary for when we enter a nested event loop on the main thread. To maintain the illusion that the main thread is blocked, we want to suspend all events on the main thread. However, we want to do so without blocking any of the worker threads.

And that's it! That's the last of the platform patches. Once I get r+ for these I should be able to quickly get r+ for the rebased frontend patches.

Is it intentional that postMessage here only accepts strings (instead of structured clones, like the other postMessage()es do)?

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #17) > Is it intentional that postMessage here only accepts strings (instead of > structured clones, like the other postMessage()es do)? Yes. The debugger protocol only uses JSON strings, so we don't need any of the structured cloning machinery.

Comment on attachment 8533175 [details] [diff] [review] Implement WorkerDebuggerGlobalScope.reportError Review of attachment 8533175 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/WorkerPrivate.h @@ +52,5 @@ > #ifdef DEBUG > struct PRThread; > #endif > > +class ReportErrorToDebuggerRunnable; This shouldn't be outside the worker namespace. ::: dom/workers/WorkerScope.cpp @@ +275,5 @@ > +{ > + JS::AutoFilename afn; > + uint32_t lineno = 0; > + JS::DescribeScriptedCaller(aCx, &afn, &lineno); > + nsString filename(NS_ConvertUTF8toUTF16(afn.get())); You can just declare NS_ConvertUTF8toUTF16 filename(afn.get()); ::: dom/workers/test/dom_worker_helper.js @@ +111,5 @@ > + }; > + if (!predicate(error)) { > + return; > + } > + dbg.removeListener(this); I don't claim to be a JS expert but I doubt that the 'this' object here is the listener. Double check that?

Comment on attachment 8533173 [details] [diff] [review] Implement WorkerDebugger.postMessage Review of attachment 8533173 [details] [diff] [review]: ----------------------------------------------------------------- I want to see this one again, so r-. ::: dom/workers/WorkerPrivate.cpp @@ +1002,5 @@ > } > return DispatchDOMEvent(aCx, aWorkerPrivate, port, false); > } > > + return DispatchDOMEvent(aCx, aWorkerPrivate, mWorkerPrivate->GlobalScope(), Why did you make this change? Everything else in this function uses aWorkerPrivate. @@ +1009,5 @@ > }; > > +class DebuggerMessageEventRunnable : public MainThreadWorkerSyncRunnable { > +public: > + nsString mMessage; This should not be public. @@ +1015,5 @@ > +public: > + DebuggerMessageEventRunnable(WorkerPrivate* aWorkerPrivate, > + nsIEventTarget* aSyncLoopTarget, > + const nsAString& aMessage) > + : MainThreadWorkerSyncRunnable(aWorkerPrivate, aSyncLoopTarget), Are you planning to ever pass in anything besides nullptr for aSyncLoopTarget? If not, drop the parameter from the constructor and hardcode nullptr here. @@ +1022,5 @@ > + } > + > +private: > + virtual bool > + IsDebuggerRunnable() const MOZ_OVERRIDE What is this? It's not in the tree already, and it's not added in this patch. This won't compile on Windows because of the MOZ_OVERRIDE annotation. @@ +3887,5 @@ > + > + nsCOMPtr<nsIRunnable> runnable = > + NS_NewRunnableMethodWithArg<nsString>(this, > + &WorkerDebugger::PostMessageToDebuggerOnMainThread, nsString(aMessage)); > + return NS_DispatchToMainThread(runnable, NS_DISPATCH_NORMAL); In practice, NS_DispatchToMainThread never fails. Go ahead and make postMessage infallible and drop propagating the nsresult here.

Comment on attachment 8533177 [details] [diff] [review] Implement WorkerDebuggerGlobalScope.enterEventLoop Review of attachment 8533177 [details] [diff] [review]: ----------------------------------------------------------------- What happens if there are debugger messages pending in the current event queue when EnterDebuggerEventLoop is called? None of those will run until the newest loop is exited. Is that a problem? ::: dom/webidl/WorkerDebuggerGlobalScope.webidl @@ +7,5 @@ > interface WorkerDebuggerGlobalScope : EventTarget { > readonly attribute WorkerGlobalScope global; > > [Throws] > + void enterEventLoop(); enterNestedEventLoop, would be clearer, I think. @@ +10,5 @@ > [Throws] > + void enterEventLoop(); > + > + [Throws] > + void leaveEventLoop(); likewise ::: dom/workers/WorkerPrivate.cpp @@ +3935,5 @@ > + return mSyncLoopTargetStack[mSyncLoopTargetStack.Length() - 1]; > +} > + > +nsresult > +WorkerDebugger::EnterDebuggerEventLoop() This can't fail, so nix the nsresult return value. @@ +3962,5 @@ > + MutexAutoLock lock(mMutex); > + > + if (mSyncLoopTargetStack.IsEmpty()) { > + return NS_ERROR_UNEXPECTED; > + } We should just assert this, and then this can be infallible too.

setTimeout has the same event loop ordering issue as comment 21, so I'm going to pause reviewing until that is answered.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #22) > setTimeout has the same event loop ordering issue as comment 21, so I'm > going to pause reviewing until that is answered. That event loop ordering issue is likely going to be a problem. I hadn't considered that, and I don't immediately know how to work around it :-( We can't simply post another runnable to the event loop that enters the nested event loop. Doing so would guarantee that all runnables that were on the queue when enterEventLoop was called are handled before the nested event loop is entered. However, that's not the behavior we want, as some of those runnables might be directed to the worker. The semantics we want is that when enterEventLoop is called, no more runnables are delivered to the worker, but any runnables that were queued for the debugger are still delivered. How would you go about this?

Can we treat debugger runnables like (a separate instance of) the control queue?

I suspect we basically want the semantics to be as similar to the main thread as we can make them, right?

I refactored the debugger runnables as we discussed on irc (i.e. put them on a separate queue). All the tests seem to work, but since you won't be able to look into these patches until the new year anyway, I'll take another pass over these patches and put them up then.

I refactored the patches based on your suggestions on dealing with nested event loops, so here's a new batch of patches for you to review. The first patch was already r+'d in bug 757133, but never landed after that. It has bitrotted a bit since then, so I think it's worthwhile if you have another look at it. Also flagging bholley for the security related stuff. Note that I addressed the review comments from the earlier r+, so other than a few nits, this should just be a rubber stamp review.

Comment on attachment 8544550 [details] [diff] [review] Implement a WorkerDebuggerGlobalScope. Apparently bholley is not available for review. Since he already r+'d the patch in the previous bug, I don't think it's worthwhile to wait for him to get back.

This patch introduces the concept of worker debugger runnables, and refactors the main event loop to take these runnables into account. The next patch will reuse the script loader to load the main debugger script. This means that ScriptExecutorRunnable can be both a normal and a debugger runnable. To avoid duplicating all the script loader machinery, I have not made a separate class hierarchy for worker debugger runnables. Instead, I've introduced a function IsDebuggerRunnable that can be overridden by each runnable.

As explained in the previous comments, this patch uses the script loader to load the initial debugger script. Note that ScriptExecutorRunnable::IsDebuggerRunnable returns a different value based on the type of script we're loading.

As explained in the previous comments, this patch uses the script loader to load the initial debugger script. Note that ScriptExecutorRunnable::IsDebuggerRunnable returns a different value based on the type of script we're loading.

This patch implements the message API for the debugger. Note that I've introduced a WorkerDebuggerRunnable helper class because debugger runnables are usually posted from the main thread, so the default assertions that WorkerRunnable maintains are invalid in this case. However, it is not necessary for debugger runnables to derive from this class (as in the case of ScriptExecutorRunnable), since MainThreadSyncRunnables already have the correct assertions anyway.

This patch is essentially the same as before.

I've changed the way nested event loops are implemented. Previously, I tried to use the existing sync loop machinery, but that led to problems because each sync loop also has its own queue. The new implementation uses a single queue for debugger runnables, which is seperate from the normal queue. The main event loop drains both the normal and the debugger queues, while nested event loops only drain the debugger queue. This allows us to suspend events in the debuggee while still being able to fire events in the debugger.

Comment on attachment 8544550 [details] [diff] [review] Implement a WorkerDebuggerGlobalScope. As far as I can tell this diff is just a combination of all your other diffs, or something. It doesn't implement WorkerDebuggerGlobalScope, that's for sure.

Comment on attachment 8544554 [details] [diff] [review] Implement worker debugger runnables Review of attachment 8544554 [details] [diff] [review]: ----------------------------------------------------------------- r- for the promise thing, but this looks pretty good. ::: dom/workers/WorkerPrivate.cpp @@ +4455,2 @@ > > + if (debuggerRunnablesPending && DebuggerGlobalScope()) { Can you actually get here without a DebuggerGlobalScope()? In other words, does the first debugger runnable not create it? @@ +4465,5 @@ > + MOZ_ALWAYS_TRUE(NS_ProcessNextEvent(mThread, false)); > + > + // Only perform the Promise microtask checkpoint on the outermost event > + // loop. Don't run it, for example, during sync XHR or importScripts. > + (void)Promise::PerformMicroTaskCheckpoint(); I suspect the debugger wants these micro task checkpoints. Otherwise promises won't behave correctly.

And, fwiw, having the correct patch for the WorkerDebuggerGlobalScope would be useful because it would allow me to apply the rest of them.

Comment on attachment 8544556 [details] [diff] [review] Implement WorkerDebugger.initialize Review of attachment 8544556 [details] [diff] [review]: ----------------------------------------------------------------- This also looks pretty good. I'd like to see these comments addressed and I'd like to test these before giving r+ though. ::: dom/workers/ScriptLoader.cpp @@ +115,5 @@ > + } > + else { > + rv = secMan->CheckLoadURIWithPrincipal(principal, uri, 0); > + NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_SECURITY_ERR); > + } Flatten these ifs, so that there's a single if level that's if (aIsDebuggerScript) { ... } else if (aIsMainScript) { ... } else { ... } @@ +789,5 @@ > .setNoScriptRval(true); > > + if (mScriptLoader.mIsDebuggerScript) { > + options.setVersion(JSVERSION_LATEST); > + } Another thing worth considering, should the debugger have a limit on the script run time? We might want to work this into the logic in RuntimeService::Init somehow, with a JSContentChromeSettings for the debugger. ::: dom/workers/ScriptLoader.h @@ +48,5 @@ > void ReportLoadError(JSContext* aCx, const nsAString& aURL, > nsresult aLoadResult, bool aIsMainThread); > > +bool LoadMainScript(JSContext* aCx, const nsAString& aScriptURL, > + bool aIsDebuggerScript); I would prefer some sort of enum type here. e.g. enum WorkerScriptType { WORKER_SCRIPT, DEBUGGER_SCRIPT, }; or something like that. boolean parameters are ugly. ::: dom/workers/WorkerPrivate.cpp @@ +853,5 @@ > + > +public: > + CompileDebuggerScriptRunnable(WorkerPrivate* aWorkerPrivate, > + const nsAString& aScriptURL) > + : WorkerRunnable(aWorkerPrivate, WorkerThreadUnchangedBusyCount), This should probably change the busy count, to match the other one. Is there a reason that it doesn't? @@ +874,5 @@ > + } > + > + virtual void > + PostDispatch(JSContext* aCx, WorkerPrivate* aWorkerPrivate, > + bool aDispatchResult) MOZ_OVERRIDE Why do you need to override Pre/PostDispatch here when the "normal" one didn't? @@ +3887,5 @@ > + > + mIsInitialized = true; > + } > + > + return NS_OK; Is there any particular reason we allow calling this multiple times?

Comment on attachment 8544559 [details] [diff] [review] Implement WorkerDebugger.postMessage Review of attachment 8544559 [details] [diff] [review]: ----------------------------------------------------------------- Modulo the extra stuff in this patch in WorkerRunnable and the two comments from the first review that aren't yet addressed, this looks good to go. ::: dom/workers/WorkerPrivate.cpp @@ +1073,1 @@ > false); why did you make this change? @@ +1075,5 @@ > }; > > +class DebuggerMessageEventRunnable : public WorkerDebuggerRunnable { > +public: > + nsString mMessage; member should be private ::: dom/workers/WorkerRunnable.h @@ +138,5 @@ > // WorkerRun() will be called on the correct thread automatically. > NS_DECL_NSIRUNNABLE > }; > > +class WorkerDebuggerRunnable : public WorkerRunnable This stuff is duplicated in this patch somehow.

Comment on attachment 8544560 [details] [diff] [review] Implement WorkerDebuggerGlobalScope.reportError Review of attachment 8544560 [details] [diff] [review]: ----------------------------------------------------------------- (This is literally a copy/paste of my last review of this patch :P) ::: dom/workers/WorkerPrivate.h @@ +55,5 @@ > } > > struct PRThread; > > +class ReportErrorToDebuggerRunnable; This shouldn't be outside the worker namespace. ::: dom/workers/WorkerScope.cpp @@ +282,5 @@ > +{ > + JS::AutoFilename afn; > + uint32_t lineno = 0; > + JS::DescribeScriptedCaller(aCx, &afn, &lineno); > + nsString filename(NS_ConvertUTF8toUTF16(afn.get())); You can just declare NS_ConvertUTF8toUTF16 filename(afn.get()); ::: dom/workers/test/dom_worker_helper.js @@ +111,5 @@ > + }; > + if (!predicate(error)) { > + return; > + } > + dbg.removeListener(this); I don't claim to be a JS expert but I doubt that the 'this' object here is the listener. Double check that?

Comment on attachment 8544572 [details] [diff] [review] Implement WorkerDebuggerGlobalScope.enterEventLoop Review of attachment 8544572 [details] [diff] [review]: ----------------------------------------------------------------- \o/ these semantics look sane to me. We should hammer out some stuff about shutdown and refactor things a bit but I think this will work. ::: dom/workers/WorkerPrivate.cpp @@ +5772,5 @@ > + > + ProcessAllControlRunnablesLocked(); > + } > + > + if (debuggerRunnablesPending) { At least this part should be factored out of DoRunLoop and put in a function this should call. We should also discuss what the shutdown handling should look like here. @@ +5799,5 @@ > +WorkerPrivate::LeaveDebuggerEventLoop() > +{ > + AssertIsOnWorkerThread(); > + > + MutexAutoLock lock(mMutex); Enter isn't locked, why does Leave need locking?

Correct patch this time.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #38) > Comment on attachment 8544554 [details] [diff] [review] > Implement worker debugger runnables > > Review of attachment 8544554 [details] [diff] [review]: > ----------------------------------------------------------------- > > r- for the promise thing, but this looks pretty good. > > ::: dom/workers/WorkerPrivate.cpp > @@ +4455,2 @@ > > > > + if (debuggerRunnablesPending && DebuggerGlobalScope()) { > > Can you actually get here without a DebuggerGlobalScope()? In other words, > does the first debugger runnable not create it? > It's possible for consumers to erroneously post a message to the debugger before initializing it, so conceivably a DebuggerMessageEventRunnable could arrive before a CompileDebuggerScriptRunnable. However, I just realized that we can disallow posting messages to an uninitialized debugger by checking the contents of the mIsDebugger field, in which case the DebuggerGlobalScope() check can go. Out of curiosity, we also check for the presence of GlobalScope() in the case of normal runnables. Wouldn't that check be redundant as well? > @@ +4465,5 @@ > > + MOZ_ALWAYS_TRUE(NS_ProcessNextEvent(mThread, false)); > > + > > + // Only perform the Promise microtask checkpoint on the outermost event > > + // loop. Don't run it, for example, during sync XHR or importScripts. > > + (void)Promise::PerformMicroTaskCheckpoint(); > > I suspect the debugger wants these micro task checkpoints. Otherwise > promises won't behave correctly. We don't even have native promises for the debugger right now, because they are not aware of the separate event queue used by the debugger. This means that promise handlers won't be called as long as we are inside a nested event loop, which is not the behavior we want. Because making the native promises debugger aware seemed like a substantial amount of work, I instead opted for an approach where we use Promise.jsm promises in the debugger. This is partially why I had to implement debugger timeouts, since Promise.jsm uses timeouts to schedule its then handlers on the event loop. In that light, I don't think that microtasks are important for the debugger *at this time*.

See comment 45 for a response to your review comments.

New patch with comments addressed. Setting a limit on script run time for the debugger seems like a bad idea. When we hit a breakpoint we enter a nested event loop, and it could take an arbitrary time for the user to do something so the debugger will continue. I need to override the dispatch hooks in CompileDebuggerScript runnable because it is dispatched from the main thread (CompileWorkerScript is not).

New patch with comments addressed. Note that WorkerDebuggerRunnable is not duplicated but a helper class that wasn't there before that implements some common methods for debugger runnables. I added a comment that hopefully makes this fact a bit clearer.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #42) > Comment on attachment 8544560 [details] [diff] [review] > Implement WorkerDebuggerGlobalScope.reportError > > Review of attachment 8544560 [details] [diff] [review]: > ----------------------------------------------------------------- > > (This is literally a copy/paste of my last review of this patch :P) > > ::: dom/workers/WorkerPrivate.h > @@ +55,5 @@ > > } > > > > struct PRThread; > > > > +class ReportErrorToDebuggerRunnable; > > This shouldn't be outside the worker namespace. If I put it in the worker namespace the friend declaration in WorkerDebugger doesn't work. I tried several things: friend class ReportErrorToDebuggerRunnable friend class mozilla::dom::workers::ReportErrorToDebuggerRunnable friend class ::mozilla::dom::workers::ReportErrorToDebuggerRunnable No matter what I use, I get compile errors telling me that ReportErrorToDebuggerRunnable is trying to access private members of WorkerDebugger to which it has no access. The only explanation I can come up with is that ReportErrorDebuggerRunnable is not defined in the namespace I think it is, so I also tried putting its definition between BEGIN_WORKER_NAMESPACE and END_WORKER_NAMESPACE. This doesn't work either. Perhaps I'm being confused here, but I have no idea why this friend declaration doesn't work if I declare ReportErrorToDebuggerRunnable in the worker namespace. I could of course make the private methods of WorkerDebugger public and avoid the need for a friend class completely, but I'd like to avoid that if at all possible. Any ideas?

Review ping

Comment on attachment 8548963 [details] [diff] [review] Implement WorkerDebugger.initialize Review of attachment 8548963 [details] [diff] [review]: ----------------------------------------------------------------- Now I understand this a bit better. You can't modify the busy count here, because the main thread may not be the parent thread of the worker (e.g. a sub-worker). You should use MainThreadSyncWorkerRunnable (with no sync target) which should override the assertions for you. Also test subworkers (and this should apply to the whole patch series). mIsMainScript && mWorkerScriptType == WorkerScript deserves a helper function, IsMainWorkerScript()?

Comment on attachment 8548976 [details] [diff] [review] Implement WorkerDebugger.postMessage Review of attachment 8548976 [details] [diff] [review]: ----------------------------------------------------------------- WorkerDebuggerRunnable belongs in the patch that implements the event queue. Can't modify busy count here either! This also needs subworker tests.

Comment on attachment 8548881 [details] [diff] [review] Implement a WorkerDebuggerGlobalScope. Review of attachment 8548881 [details] [diff] [review]: ----------------------------------------------------------------- This is pretty close. First it needs to be rebased over bug 1092102. I had to do that to apply the rest of the patch series, so I'll attach the rebased patch later. Second, see the comments in the IDL. ::: dom/webidl/WorkerDebuggerGlobalScope.webidl @@ +2,5 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this file, > + * You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +[Global=(Worker), Exposed=Worker] This should definitely not be exposed in Worker. If we want other DOM objects (e.g. XHR available), it should be [Global=(Worker,WorkerDebugger), Exposed=WorkerDebugger]. If we don't, it should be [Global=(WorkerDebugger), Exposed=WorkerDebugger]. If you do want these other things I'd like you to run the XHR tests in the debugger scope, at least.

Comment on attachment 8548903 [details] [diff] [review] Implement worker debugger runnables Review of attachment 8548903 [details] [diff] [review]: ----------------------------------------------------------------- r=me

A partial rebase of this (that I used for reviewing) is available at https://github.com/khuey/gecko-dev/compare/ejpbruel-review

(In reply to Eddy Bruel [:ejpbruel] from comment #49) > (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #42) > > Comment on attachment 8544560 [details] [diff] [review] > > Implement WorkerDebuggerGlobalScope.reportError > > > > Review of attachment 8544560 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > (This is literally a copy/paste of my last review of this patch :P) > > > > ::: dom/workers/WorkerPrivate.h > > @@ +55,5 @@ > > > } > > > > > > struct PRThread; > > > > > > +class ReportErrorToDebuggerRunnable; > > > > This shouldn't be outside the worker namespace. > > If I put it in the worker namespace the friend declaration in WorkerDebugger > doesn't work. I tried several things: > > friend class ReportErrorToDebuggerRunnable > friend class mozilla::dom::workers::ReportErrorToDebuggerRunnable > friend class ::mozilla::dom::workers::ReportErrorToDebuggerRunnable > > No matter what I use, I get compile errors telling me that > ReportErrorToDebuggerRunnable is trying to access private members of > WorkerDebugger to which it has no access. The only explanation I can come up > with is that ReportErrorDebuggerRunnable is not defined in the namespace I > think it is, so I also tried putting its definition between > BEGIN_WORKER_NAMESPACE and END_WORKER_NAMESPACE. This doesn't work either. > > Perhaps I'm being confused here, but I have no idea why this friend > declaration doesn't work if I declare ReportErrorToDebuggerRunnable in the > worker namespace. I could of course make the private methods of > WorkerDebugger public and avoid the need for a friend class completely, but > I'd like to avoid that if at all possible. > > Any ideas? That's bizarre, but I have to admit that I don't care enough about it to try to figure it out. Leaving it as is is ok.

I don't know what bug you want me to rebase this over. You mentioned bug 1092102 in comment #53, but that's this bug. I did a pull from master and rebased the patches on top of that, but didn't get any conflicts, so I'm not sure if that's what you did. In any case, here is the rebased patch with comments addressed. We don't want to expose things like XHR to the debugger at the moment. XHR doesn't play nicely with nested event loops, because it doesn't know about the debugger queue, so it wouldn't even work properly. Note that I had to expose EventTarget to the debugger as well, since WorkerDebuggerGlobalScope inherits from it. To avoid having to expose WorkerGlobalScope and everything that pulls in transitively as well, I refactored the global property on WorkerGlobalScope to return a JSObject instead. This does not affect the debugger API in any way. Because this is the first patch in the queue, this is blocking me from landing anything else. Please let me know asap if I need to rebase it on top of something else, or if there's anything else wrong with it.

Moved the implementation of WorkerDebuggerRunnable from the WorkerDebugger.postMessage patch to this one, as you requested.

Added tests for subworkers and a IsMainWorkerScript helper function. Note that CompileDebuggerScriptRunnable inherits from WorkerDebuggerRunnable, which was added in the previous patch, and no longer changes the busy count. While I was at it, I took the liberty to improve some of the existing tests for the WorkerDebuggerManager and WorkerDebugger as well (simplified some of the helper functions in dom_worker_helpers.js, and added more info statements to the test).

Moved the implementation of WorkerDebuggerRunnable to the worker debugger runnable patch, and added subworker tests. Note that since WorkerDebuggerRunnable no longer changes the busy count, DebuggerMessageEventRunnable doesn't either.

Comment on attachment 8564996 [details] [diff] [review] Implement worker debugger runnables Review of attachment 8564996 [details] [diff] [review]: ----------------------------------------------------------------- I didn't look at this again, I assumed you copy/pasted correctly.

I want to apply and test the other two patches, so I'll resume reviewing tomorrow.

Comment on attachment 8564998 [details] [diff] [review] Implement WorkerDebugger.initialize Review of attachment 8564998 [details] [diff] [review]: ----------------------------------------------------------------- This patch doesn't pass tests, at least on my machine. 352 INFO TEST-UNEXPECTED-FAIL | dom/workers/test/test_WorkerDebugger.initialize.xul | uncaught exception - ReferenceError: global is not defined at chrome://mochitests/content/chrome/dom/workers/test/WorkerDebugger.initialize_debugger.js:3 ::: dom/workers/WorkerPrivate.cpp @@ +904,5 @@ > + > +public: > + CompileDebuggerScriptRunnable(WorkerPrivate* aWorkerPrivate, > + const nsAString& aScriptURL) > + : WorkerRunnable(aWorkerPrivate, WorkerThreadModifyBusyCount), You didn't update this when you changed the inheritance, so this doesn't actually build.

Sorry, turns out I *don't* know how to copy/paste correctly. I forgot to copy a function definition in the *.cpp file. I've updated the patch. Make sure you base subsequent patches on top of this one.

This patch compiles on my machine. Tests seem to be passing as well.

You said that the WorkerDebugger.initialize test fails on your machine because global is not defined in the debugger script. This could be related to the WorkerDebuggerGlobalScope patch. Are you sure that is up to date? I've made sure that the patch attached here is identical to the one in my patch queue.

After rebasing, the WorkerDebugger.initialize tests fail for me as well :-S It looks like none of the properties/method I defined on WorkerDebuggerGlobalScope are defined in the debugger script. Did something recently change in the bindings and/or worker code that could cause this to happen? I still don't really understand how the webidl Expose mechanism works, are we sure that I am using that correctly?

(In reply to Eddy Bruel [:ejpbruel] from comment #67) > After rebasing, the WorkerDebugger.initialize tests fail for me as well :-S > > It looks like none of the properties/method I defined on > WorkerDebuggerGlobalScope are defined in the debugger script. Did something > recently change in the bindings and/or worker code that could cause this to > happen? I still don't really understand how the webidl Expose mechanism > works, are we sure that I am using that correctly? No, you're not. Apparently I don't understand it correctly. The correct incantatations appear to be: [Global=(Worker,WorkerDebugger),Exposed=WorkerDebugger] Without the 'Worker' bit you don't get any properties (wtf?) but even with it you don't get the properties from WorkerGlobalScope, since you don't inherit from it. Changing that passes the test.

Try push for the WorkerDebuggerGlobalScope patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=90c6b39a0617

Oops, that try run uses the wrong test suites. Here is the correct one: https://treeherder.mozilla.org/#/jobs?repo=try&revision=192d57e95ce2

Try push for the worker debugger runnables patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=57a3ea4391b2

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #68) > (In reply to Eddy Bruel [:ejpbruel] from comment #67) > > After rebasing, the WorkerDebugger.initialize tests fail for me as well :-S > > > > It looks like none of the properties/method I defined on > > WorkerDebuggerGlobalScope are defined in the debugger script. Did something > > recently change in the bindings and/or worker code that could cause this to > > happen? I still don't really understand how the webidl Expose mechanism > > works, are we sure that I am using that correctly? > > No, you're not. Apparently I don't understand it correctly. The correct > incantatations appear to be: > > [Global=(Worker,WorkerDebugger),Exposed=WorkerDebugger] > > Without the 'Worker' bit you don't get any properties (wtf?) but even with > it you don't get the properties from WorkerGlobalScope, since you don't > inherit from it. > > Changing that passes the test. Huh. That's weird. Perhaps we should check with bz if we're actually doing the right thing here? In the meantime, you should be able to review this patch with this change applied, no? (and the postMessage patch as well). Boris, can you explain to us why we need to add 'Worker' to the Global attribute of WorkerDebuggerGlobalScope? We use this interface for the global object of the worker debugger, which runs in a separate scope from the main worker script when initialized. I'd expect to be able to just say Global=(WorkerDebugger) and Exposed=WorkerDebugger. However, when we do that, none of the properties on WorkerDebuggerGlobalScope are available in the debugger script. Is there some special casing going on here in the DOM bindings? Or are we missing something?

Updated the patch to add the 'Worker' bit, so we won't accidentally base subsequent patches on top of an outdated version of this. Let me know if everything compiles/runs properly for you now.

One of the changes I forgot to do in the WorkerDebugger.initialize patch ended up in the WorkerDebugger.postMessage patch, so here's an updated version of that as well.

You have a "'workers': True" descriptor in Bindings.conf, right? In our codegen, we skip generating any code for members of "worker" descriptors that are not exposed in any worker, where "is exposed in any worker" is defined as "exposed in things that have Worker as one of their global names". The idea is that if you have a worker-specific descriptor, then only things that are exposed in workers are going to be visible in JS anyway, and it's silly to force C++ implementations to exist for stuff that won't be exposed (we used to require it, and added this check just so we could remove the pointless and confusing C++ bits). So you have three options: 1) Don't mark the descriptor as a worker-specific descriptor. 2) Add "Worker" to the set of global names for this global. 3) Change the implementation of isExposedInAnyWorker.

So given that you don't want any of the WorkerPrivate::RegisterBindings bits (except maybe profiling functions?) and that this thing is not in any way a "Worker" in terms of what's exposed, I think #1 is the right approach, fwiw. I checked, and this descriptor in Bindings.conf: 'WorkerDebuggerGlobalScope': { 'headerFile': 'mozilla/dom/WorkerScope.h', 'nativeType': 'mozilla::dom::workers::WorkerDebuggerGlobalScope', 'implicitJSContext': [ 'dump', 'global', ], }, seems to compile fine and generates the properties and whatnot.

(In reply to Boris Zbarsky [:bz] from comment #76) > So given that you don't want any of the WorkerPrivate::RegisterBindings bits > (except maybe profiling functions?) and that this thing is not in any way a > "Worker" in terms of what's exposed, I think #1 is the right approach, fwiw. > I checked, and this descriptor in Bindings.conf: > > 'WorkerDebuggerGlobalScope': { > 'headerFile': 'mozilla/dom/WorkerScope.h', > 'nativeType': 'mozilla::dom::workers::WorkerDebuggerGlobalScope', > 'implicitJSContext': [ > 'dump', 'global', > ], > }, > > seems to compile fine and generates the properties and whatnot. The WorkerDebuggerGlobalScope patch compiles fine, but the worker debugger runnables patch does not. The problem is that we use UNWRAP_WORKER_OBJECT in WorkerPrivate.cpp to WorkerScope.cpp to unwrap the worker debugger global. If we don't set the isWorker flag for WorkerDebuggerGlobalScope in Bindings.h, we can no longer do that. I have no idea if this is supposed to be safe, but tried using UNWRAP_OBJECT instead. That seemed to make the worker debugger runnable patch compile, but when I then run the test in the WorkerDebugger.initialize patch, it e fails spectacularly :-( It doesn't even fail consistently. The first time I ran it, I got an infinite recursion that looks like this: frame #1: 0x000000010740ec2b XUL`JS_WrapObject(cx=0x00000001331f4840, objp=JS::MutableHandleObject at 0x0000000139c96208) + 139 at jsapi.cpp:853 * frame #2: 0x0000000102d1a46b XUL`nsGlobalWindow::OuterObject(aCx=0x00000001331f4840, aObj=Handle<JSObject *> at 0x0000000139c96290) + 347 at nsGlobalWindow.cpp:2013 frame #3: 0x0000000106dc9e63 XUL`js::GetOuterObject(cx=0x00000001331f4840, obj=JS::HandleObject at 0x0000000139c962d0) + 83 at jsobj.h:1023 frame #4: 0x000000010740f0b2 XUL`JSCompartment::wrap(this=0x00000001388d1000, cx=0x00000001331f4840, obj=JS::MutableHandleObject at 0x0000000139c96600, existingArg=JS::HandleObject at 0x0000000139c965f8) + 1074 at jscompartment.cpp:372 frame #5: 0x000000010740ec2b XUL`JS_WrapObject(cx=0x00000001331f4840, objp=JS::MutableHandleObject at 0x0000000139c96668) + 139 at jsapi.cpp:853 frame #6: 0x0000000102d1a46b XUL`nsGlobalWindow::OuterObject(aCx=0x00000001331f4840, aObj=Handle<JSObject *> at 0x0000000139c966f0) + 347 at nsGlobalWindow.cpp:2013 frame #7: 0x0000000106dc9e63 XUL`js::GetOuterObject(cx=0x00000001331f4840, obj=JS::HandleObject at 0x0000000139c96730) + 83 at jsobj.h:1023 frame #8: 0x000000010740f0b2 XUL`JSCompartment::wrap(this=0x00000001388d1000, cx=0x00000001331f4840, obj=JS::MutableHandleObject at 0x0000000139c96a60, existingArg=JS::HandleObject at 0x0000000139c96a The second time around, I got an assertion error here: * thread #1: tid = 0x415790, 0x00000001075e5435 XUL`js::gc::ArenaHeader::allocated(this=0x000000011be98000) const + 53 at Heap.h:574, queue = 'com.apple.main-thread', stop reason = signal SIGSTOP frame #0: 0x00000001075e5435 XUL`js::gc::ArenaHeader::allocated(this=0x000000011be98000) const + 53 at Heap.h:574 571 inline Chunk *chunk() const; 572 573 bool allocated() const { -> 574 MOZ_ASSERT(allocKind <= size_t(FINALIZE_LIMIT)); 575 return allocKind < size_t(FINALIZE_LIMIT); 576 } 577 The third time I ran it, the test passed, but I got a memory leak. I have no idea what is going on here, and I don't understand the DOM bindings code well enough to figure this out. We are currently using option 2, which is to add Worker to the globals attribute of WorkerDebuggerGlobalScope, and this seems to just work, so I'm tempted to leave things as is. If you strongly feel that this is not the right thing to do, let me know, and we can look into this, but I'm going to need some serious help in that case.

> I have no idea if this is supposed to be safe, but tried using UNWRAP_OBJECT instead. UNWRAP_OBJECT is exactly the same thing as UNWRAP_WORKER_OBJECT, except for the way it derives the binding name from the interface name. So yes, replacing UNWRAP_WORKER_OBJECT with UNWRAP_OBJECT should be just fine. > The first time I ran it, I got an infinite recursion that looks like this: Just to check: you _did_ recompile the whole tree, right? > and I don't understand the DOM bindings code well enough to figure this out. I'm happy to help, fwiw. Post the patch that I'd need to apply? > We are currently using option 2, I feel pretty strongly that this is wrong. This global is in no way a worker global in the sense that bindings think of them. I'm happy to help you figure out what's going on...

I've made a patch that should allow you to reproduce the problem locally. This patch combines the WorkerDebuggerGlobalScope, worker debugger runnables and WorkerDebugger.initialize. I've removed the worker attribute from the Bindings.conf entry for WorkerDebuggerGlobalScope, as you suggested, and changed UNWRAP_WORKER_OBJECT to UNWRAP_OBJECT where applicable. If you apply this patch on top of mozilla-central, and then try to run test_WorkerDebugger.initialize, you should be able to consistently get a test failure.

Comment on attachment 8567964 [details] [diff] [review] patch I dunno what you diffed this from and to, but this patch clearly has all sorts of random stuff in it and does not apply on top of m-c (rev 9b077c6f3d02).

This is what happens when you work on multiple repositories at the same time. I still had a commit ref from another repo on my clipboard.

Comment on attachment 8567979 [details] [diff] [review] patch Thanks, that helps. So I think the problem is that when codegen sees a descriptor which isGlobal() but not workers, it assumes it wants the JSClass hooks for Window (including nsGlobalWindow::OuterObject, which explains your infinite recursion above, and JS_ObjectToOuterObject as the thisObject hook). If I change the check in CGDOMJSClass.define to explicitly exclude WorkerDebuggerGlobalScope, the test passes. Of course we still get some code generated for Xray bits that's dead code.... so maybe we do want a worker descriptor here after all. Let me think a bit about what the right approach here is.

OK, so given bug 888600 and bug 1135220 this assumption that "not worker means window" for IDL globals is bunk. I filed bug 1135792 on that. In general, there seems to be this baked-in assumption that globals come in two buckets "workers" and "window". That's silly, and we're going to fix that. So I think what you should do is to work on top of bug 1135792 and use a non-worker descriptor. We'll generate some Xray bits, but I filed bug 1135810 to fix that; it need not block you landing.

FYI, I'm waiting for a patch that implements #1 from comment 75 before proceeding on the review.

Here is an updated patch that implements #1 from comment 75.

Here's a rebased version of the worker debugger runnables patch. It *should* be unchanged from the previous one, so this only requires a rubber stamp review. I'm mainly putting it up to make sure the subsequent patches will apply cleanly. I'll put rebased versions up for those too.

Try push for the updated WorkerDebuggerGlobalScope and worker-debugger-runnables patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f21200aa9dfd https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ccb43d433cf

If you wanted to, you could "wantXrays": False this interface now that I've fixed bug 1135810.

(In reply to Not doing reviews right now from comment #90) > If you wanted to, you could "wantXrays": False this interface now that I've > fixed bug 1135810. Cool, let's do that in a separate patch. I'd like to avoid yet another review pass for these patches. Thanks once again for helping me out so quickly with this!

Comment on attachment 8569831 [details] [diff] [review] WorkerDebuggerGlobalScope Review of attachment 8569831 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/WorkerScope.cpp @@ +515,5 @@ > +IsWorkerGlobal(JSObject* object) > +{ > + nsIGlobalObject* globalObject = nullptr; > + UNWRAP_WORKER_OBJECT(WorkerGlobalScope, object, globalObject); > + return !!globalObject; These should be return NS_SUCCEEDED(UNWRAP_....) && !!globalObject;. You can also skip initializing globalObject if you do that. @@ +523,5 @@ > +IsDebuggerGlobal(JSObject* object) > +{ > + nsIGlobalObject* globalObject = nullptr; > + UNWRAP_OBJECT(WorkerDebuggerGlobalScope, object, globalObject); > + return !!globalObject; ibid ::: dom/workers/WorkerScope.h @@ +248,5 @@ > + } > + > + virtual bool > + WrapGlobalObject(JSContext* aCx, > + JS::MutableHandle<JSObject*> aReflector); MOZ_OVERRIDE

Comment on attachment 8569833 [details] [diff] [review] WorkerDebugger.initialize Review of attachment 8569833 [details] [diff] [review]: ----------------------------------------------------------------- This patch leaks. I think it's likely that it's just the added tests that leak, and the bug is somewhere else. ::: dom/webidl/WorkerDebuggerGlobalScope.webidl @@ +2,5 @@ > /* This Source Code Form is subject to the terms of the Mozilla Public > * License, v. 2.0. If a copy of the MPL was not distributed with this file, > * You can obtain one at http://mozilla.org/MPL/2.0/. */ > > +[Global=(Worker,WorkerDebugger), Exposed=WorkerDebugger] Why is this change here? ::: dom/workers/nsIWorkerDebugger.idl @@ +27,5 @@ > > readonly attribute nsIDOMWindow window; > > + [implicit_jscontext] > + void initialize(in DOMString url); needs a uuid bump

Comment on attachment 8569834 [details] [diff] [review] WorkerDebugger.postMessage Review of attachment 8569834 [details] [diff] [review]: ----------------------------------------------------------------- This doesn't build on Windows because of the PostMessageA/W stuff. You should do the binaryname(PostMessageMoz) thing we do on Window. ::: dom/workers/nsIWorkerDebugger.idl @@ +6,5 @@ > interface nsIWorkerDebuggerListener : nsISupports > { > void onClose(); > + > + void onMessage(in DOMString message); iid change. @@ +32,5 @@ > [implicit_jscontext] > void initialize(in DOMString url); > > + [implicit_jscontext] > + void postMessage(in DOMString message); here too

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #93) > Comment on attachment 8569833 [details] [diff] [review] > WorkerDebugger.initialize > > Review of attachment 8569833 [details] [diff] [review]: > ----------------------------------------------------------------- > > This patch leaks. I think it's likely that it's just the added tests that > leak, and the bug is somewhere else. > So no action required here, right?

(In reply to Eddy Bruel [:ejpbruel] from comment #95) > (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #93) > > Comment on attachment 8569833 [details] [diff] [review] > > WorkerDebugger.initialize > > > > Review of attachment 8569833 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > This patch leaks. I think it's likely that it's just the added tests that > > leak, and the bug is somewhere else. > > > > So no action required here, right? Well you can't land the patch because it will turn the trees orange. I would think that means action is required ;) Run ./mach mochitest-chrome dom/workers on a debug build with that patch.

I had to rebase this the WorkerDebuggerGlobalScope patch because of some changes to the wrapper API. Made another try push for it, just in case: https://treeherder.mozilla.org/#/jobs?repo=try&revision=200611f095b4 Try run looks good, so landed on inbound with final comments by khuey addressed: https://hg.mozilla.org/integration/mozilla-inbound/rev/54ea9cbf8439

Try run for the worker debugger runnables patch looks good too: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f65fb0ef6235 Landed on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/b06982ec6797

https://hg.mozilla.org/mozilla-central/rev/54ea9cbf8439 https://hg.mozilla.org/mozilla-central/rev/b06982ec6797

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #93) > Comment on attachment 8569833 [details] [diff] [review] > WorkerDebugger.initialize > > Review of attachment 8569833 [details] [diff] [review]: > ----------------------------------------------------------------- > > This patch leaks. I think it's likely that it's just the added tests that > leak, and the bug is somewhere else. > > ::: dom/webidl/WorkerDebuggerGlobalScope.webidl > @@ +2,5 @@ > > /* This Source Code Form is subject to the terms of the Mozilla Public > > * License, v. 2.0. If a copy of the MPL was not distributed with this file, > > * You can obtain one at http://mozilla.org/MPL/2.0/. */ > > > > +[Global=(Worker,WorkerDebugger), Exposed=WorkerDebugger] > > Why is this change here? > > ::: dom/workers/nsIWorkerDebugger.idl > @@ +27,5 @@ > > > > readonly attribute nsIDOMWindow window; > > > > + [implicit_jscontext] > > + void initialize(in DOMString url); > > needs a uuid bump I cannot reproduce this leak. I built Firefox using the following flags: ac_add_options --enable-debug ac_add_options --disable-optimize mk_add_options MOZ_MAKE_FLAGS=-j4 mk_add_options MOZ_OBJDIR=obj-debug Then ran the test 3 times with the following command: ./mach mochitest-chrome dom/workers/tests/test_WorkerDebugger.initialize Each time, the leak check tells me: TEST-INFO | leakcheck | default process: leak threshold set at 0 bytes TEST-INFO | leakcheck | plugin process: leak threshold set at 0 bytes TEST-INFO | leakcheck | tab process: leak threshold set at 100000 bytes TEST-INFO | leakcheck | geckomediaplugin process: leak threshold set at 20000 bytes == BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS, default process 91763 |<----------------Class--------------->|<-----Bytes------>|<----------------Objects---------------->|<--------------References-------------->| Per-Inst Leaked Total Rem Mean StdDev Total Rem Mean StdDev 0 TOTAL 29 0 1133003 0 ( 6113.07 +/- 10608.64) 1638157 0 ( 4627.54 +/- 8872.25) nsTraceRefcnt::DumpStatistics: 1141 entries TEST-PASS | leakcheck | default process: no leaks detected! What steps did you use to produce this leak?

See comment 96? Alternatively push it to try?

Try push to see if I can reproduce those leaks that Kyle reported: https://treeherder.mozilla.org/#/jobs?repo=try&revision=15d6e6f3d6c1 Also a friendly note that I will be on a city trip to Stockholm with my wife until tuesday, so I won't be able to look into this in depth until then.

Ok, so there definitely is some leak going on, but I can only reproduce it on try, and it's not clear to me from the try logs what test is causing the leak. I have no idea how WorkerDebugger.initialize could cause this, or how I can figure out what's causing the leak, so I'm going to need help with this. Kyle, can you spare some time next week to help me hunt down this leak?

ejpbruel is going to schedule something.

New patch with comments and leak addressed, as discussed on irc.

Five more patches to go after this one.

Try run for the WorkerDebugger.initialize patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=705325498151

Try run for the WorkerDebugger.postMessage patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=62730f6d2af4

Note that the next patch implements WorkerDebuggerGlobalScope.setTimeout. I'm in the process of simplifying that, since I realised that the debugger server actually only ever needs setTimeout(0) (or setImmediate). I can have that patch up by tomorrow, or friday at the very latest. Is that ok?

You can take all the time you want. Encouraging you to get the patches up is for your benefit, not mine ;)

Comment on attachment 8575987 [details] [diff] [review] WorkerDebugger.initialize Review of attachment 8575987 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/WorkerDebuggerManager.h @@ +72,5 @@ > + nsRefPtr<WorkerDebuggerManager> manager = > + WorkerDebuggerManager::GetOrCreateService(); > + if (!manager) { > + return NS_ERROR_FAILURE; > + } This should just be Get, there's no reason to create it just to clear listeners.

Somehow you generated these most recent patches with 3 lines of context. Please don't do that again.

Comment on attachment 8575990 [details] [diff] [review] WorkerDebuggerGlobalScope.reportError Review of attachment 8575990 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/WorkerPrivate.cpp @@ +4372,5 @@ > + { > + MutexAutoLock lock(mMutex); > + > + listeners.AppendElements(mListeners); > + } We keep doing this and probably should factor it out into a function. Maybe something like nsTArray<nsCOMPtr<nsIWorkerDebuggerListener>>&& LockAndCopyListeners() const; ::: dom/workers/nsIWorkerDebugger.idl @@ +7,5 @@ > { > void onClose(); > > + void onError(in DOMString filename, in unsigned long lineno, > + in DOMString message); iid change.

Note the patch I am refactoring, but I realized that the next patch doesn't actually depend on that one, so I can put it up already.

Comment on attachment 8576041 [details] [diff] [review] WorkerDebuggerGlobalScope.enterEventLoop Review of attachment 8576041 [details] [diff] [review]: ----------------------------------------------------------------- Not sure why this is from a different bugzilla account. What happens(/should happen) if the worker tries to shutdown while the debugger is active? Right now we won't shut down without the consent of the debugger. Whatever behavior we want needs tests. This also needs subworker tests.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #115) > Comment on attachment 8576041 [details] [diff] [review] > WorkerDebuggerGlobalScope.enterEventLoop > > Review of attachment 8576041 [details] [diff] [review]: > ----------------------------------------------------------------- > > Not sure why this is from a different bugzilla account. > > What happens(/should happen) if the worker tries to shutdown while the > debugger is active? Right now we won't shut down without the consent of the > debugger. > I think that would be the simplest thing to do here. The debugger will always give its consent unless it is paused on something like a breakpoint. If that's the case, I think its fair to say that we are paused for a reason, and the user is trying to debug something. That the worker could suddenly decide to shut down in that case seems undesirable. Chiming in jimb to get his opinion on this. > Whatever behavior we want needs tests. This also needs subworker tests. What exactly do you want subworker tests for? The shutdown behavior? If so, sure. If not, it doesn't seem particularly useful to write subworker tests for nested event loops, since the behavior is the same in both cases.

The debugger server doesn't need to be able to veto a shutdown, but it does need notification and a chance to run its event loop, for an indefinite amount of time. So it would suffice, for example, to let the debug server supply a callback function that Gecko invokes when the worker is going to shut down. The server could spin up its event loop if it wanted to, and then as soon as it returns or throws, the worker would disappear. (... I hope we have good logging facilities, so that errors don't get eaten.)

(In reply to Jim Blandy :jimb from comment #117) > The debugger server doesn't need to be able to veto a shutdown, but it does > need notification and a chance to run its event loop, for an indefinite > amount of time. So it would suffice, for example, to let the debug server > supply a callback function that Gecko invokes when the worker is going to > shut down. The server could spin up its event loop if it wanted to, and then > as soon as it returns or throws, the worker would disappear. > > (... I hope we have good logging facilities, so that errors don't get eaten.) I'm interpreting this as that debugger event loops should indeed block the worker from shutting down. Is that correct?

The previous try push for WorkerDebugger.initialize was suffering from infra failures, so I did a second one: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0fb3f506f506 That try push looks good, so landed on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/757de8e339e0

Second try push for WorkerDebugger.postMessage is showing build failures on Windows. The compiler claims that WorkerDebugger::PostMessage is not defined, even though its defined in the IDL. On my OSX machine, the corresponding generated header file looks as follows: https://pastebin.mozilla.org/8826069 Which looks like it matches the signature of my implementation. What is going on here? Why is the compiler complaining?

Forgot to attach the relevant try push for comment 120: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b6006c81a5a

This patch is a refactor of WorkerDebuggerGlobalScope.setTimeout. I realized we don't actually need full timer support for the debugger, we just need a way to schedule a callback on the event loop, for which a setImmediate like API is sufficient.

The build failure on Windows is probably what I mentioned in comment 94.

(In reply to Eddy Bruel [:ejpbruel] from comment #118) > I'm interpreting this as that debugger event loops should indeed block the > worker from shutting down. Is that correct? Yes, that's what makes sense to me.

Comment on attachment 8577268 [details] [diff] [review] WorkerDebuggerGlobalScope.createSandbox Review of attachment 8577268 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsWrapperCache.h @@ +12,5 @@ > #include "js/Id.h" // must come before js/RootingAPI.h > #include "js/Value.h" // must come before js/RootingAPI.h > #include "js/RootingAPI.h" > #include "js/TracingAPI.h" > +#include "jsapi.h" Mmm, no. This belongs somewhere else. ::: dom/workers/WorkerScope.cpp @@ +681,5 @@ > +WorkerDebuggerGlobalScope::LoadSubScript( > + JSContext* aCx, > + const nsAString& aURL, > + const Optional<JS::Handle<JSObject*>>& aSandbox, > + ErrorResult& aRv) This is ... interesting ... indentation.

New try push for the WorkerDebugger.postMessage patch (Windows only), with comment 94 addressed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8db83931f0fd

Apparently I misunderstood how binaryname is supposed to be used. Here's another attempt: https://treeherder.mozilla.org/#/jobs?repo=try&revision=60d21cc4ff5f

Finally, try push looks green. Pushing to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/7a5061645ba1

This patch renames Suspend/Resume to Freeze/Thaw, so we can reuse Suspend/Resume for methods that temporarily suspend worker events from being fired on the main thread, without actually stopping the worker from running (like Freeze does). Recall that we need to suppress all events from firing to create the illusion that the main thread is paused (even though in reality the debugger runs in the same thread).

And finally, this is the last patch! \O/ This patch implements a isFrozen property and freeze/thaw events on WorkerDebugger. This allows us to detect when workers are being frozen/thawed. This is necessary because workers are frozen when they go into the bfcache, which happens when we navigated to another tab, and we generally don't want to show workers that do not belong to the current page.

Try run for the WorkerDebuggerGlobalScope.reportError patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c35825c0702a

Try run looks good, pushing to mozilla-inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/6f837658dea2

Try run for the WorkerDebuggerGlobalScope.enterEventLoop patch (with subworker and shutdown tests added): https://treeherder.mozilla.org/#/jobs?repo=try&revision=d23dbafac816

Try run looks good, pushing to mozilla-inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/3b30137b0f6a

Try run for the WorkerDebuggerGlobalScope.setImmediate patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=322966fe2546

New patch for WorkerDebuggerGlobalScope.createSandbox, with comments addressed.

https://hg.mozilla.org/mozilla-central/rev/6f837658dea2 https://hg.mozilla.org/mozilla-central/rev/3b30137b0f6a

Comment on attachment 8581798 [details] [diff] [review] Rename Suspend/Resume to Freeze/Thaw Review of attachment 8581798 [details] [diff] [review]: ----------------------------------------------------------------- Your commit message for this should mention removing the unnecessary SynchronizeAndResume queuing bit.

Comment on attachment 8581803 [details] [diff] [review] WorkerDebugger.isFrozen Review of attachment 8581803 [details] [diff] [review]: ----------------------------------------------------------------- How do you plan to actually use this in the debugger?

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) (PTO starting 4/1, back 4/8) from comment #141) > Comment on attachment 8581803 [details] [diff] [review] > WorkerDebugger.isFrozen > > Review of attachment 8581803 [details] [diff] [review]: > ----------------------------------------------------------------- > > How do you plan to actually use this in the debugger? We display a list of active workers for the current tab in the debugger UI. When the tab navigates to another page, the previous page is moved into the bfcache, and any workers on the page are frozen as a result. This will trigger the freeze event, in response to which the workers will be removed from the list of active workers. When we navigate back to the previous page, it is moved out of the bfcache, and the workers are thawed, triggering the thawed event, in response to which the workers will once again be shown in the list of active workers.

Try push for the WorkerDebuggerGlobalScope.createSandbox patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=406652a90746

Comment on attachment 8581803 [details] [diff] [review] WorkerDebugger.isFrozen Review of attachment 8581803 [details] [diff] [review]: ----------------------------------------------------------------- I worry that this will be inherently racy, because we're waiting for us to round-trip to the worker thread and back before setting the frozen flag on the debugger (when freezing/thawing is always initiated from the main thread). But perhaps that's the behavior you want. It's easy enough to change if not.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) (PTO starting 4/1, back 4/8) from comment #144) > Comment on attachment 8581803 [details] [diff] [review] > WorkerDebugger.isFrozen > > Review of attachment 8581803 [details] [diff] [review]: > ----------------------------------------------------------------- > > I worry that this will be inherently racy, because we're waiting for us to > round-trip to the worker thread and back before setting the frozen flag on > the debugger (when freezing/thawing is always initiated from the main > thread). But perhaps that's the behavior you want. It's easy enough to > change if not. The frozen/thawed event should be understood as that the worker did freeze/thaw at some point *before* the event triggered. Taking the frozen event as an example, there is no guarantee that the worker has not been thawed again by the time the event triggers. The only guarantee you get is that whenever the worker changes state on the worker thread, a message is dispatched to the debugger, so if the worker has already been thawed by the time the frozen event triggers, you *will* eventually get a thawed event as well. In other words, the debugger's knowledge of the frozen state of the worker may lag slightly on the actual state, but will always converge eventually. One consequence of this is that there is a slight lag in the UI with regard to the list of active workers. This was barely perceptible in the prototype I wrote, however, so I don't consider that a problem.

Try push for the WorkerDebuggerGlobalScope.setImmediate patch looks good, landing on mozilla-inbound with some last minute changes to some info strings in the tests: https://hg.mozilla.org/integration/mozilla-inbound/rev/cab512346eee

The try push for the WorkerDebuggerGlobalScope.createSandbox patch looked fine overall, but revealed a memory leak in the test due to not properly waiting on a promise to resolve. Here's a new try push with that issue resolved: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6677b90d3cbe

This might be overkill, but here's a try patch for the rename patch as well: https://treeherder.mozilla.org/#/jobs?repo=try&revision=41f63e941495

(In reply to Eddy Bruel [:ejpbruel] from comment #146) > Try push for the WorkerDebuggerGlobalScope.setImmediate patch looks good, > landing on mozilla-inbound with some last minute changes to some info > strings in the tests: > https://hg.mozilla.org/integration/mozilla-inbound/rev/cab512346eee The "IsDebuggerRunnable" impl here triggered this clang 3.6 build warning, treated as an error in warnings-as-errors builds: { WorkerPrivate.cpp:1693:3: error: 'IsDebuggerRunnable' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override] } I'm going to push a followup to add the missing "override" annotation to fix this. (using blanket r+ that ehsan granted me for fixes of this sort over in bug 1126447 comment 2)

Landed followup to add "override": https://hg.mozilla.org/integration/mozilla-inbound/rev/2c8dacaae28c

(In reply to Daniel Holbert [:dholbert] from comment #150) > Landed followup to add "override": > https://hg.mozilla.org/integration/mozilla-inbound/rev/2c8dacaae28c Thanks for not only reporting my problem, but even fixing it. Wow!

The try run for the WorkerDebuggerGlobalScope.createSandbox patch is almost there, but still show a compile error on Linux64 static analysis builds due to a missing 'explicit' keyword in front of a constructor. Here's a partial rerun of that try push to make sure that it now compiles properly: https://treeherder.mozilla.org/#/jobs?repo=try&revision=56240685bc60

Try push for the WorkerDebuggerGlobalScope.createSandbox patch looks good, landing on mozilla-inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/71abbf190d53

The try push for the rename patch was causing a crash in a test because I replaced a call to SynchronizeAndResume with a call to Thaw (previously Resume). The former contains a check for when the worker was created when the window was already suspend (maybe during a sync XHR), but the latter does not. Replacing the assertion in Thaw with the check from SynchronizeAndResume should solve the problem. Here's a new try run for the rename patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=11fc9e9b246d

sorry backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=8274006&repo=mozilla-inbound

Doh, thats because I landed the old patch. Pushing to mozilla-inbound again, with the correct patch this time: https://hg.mozilla.org/integration/mozilla-inbound/rev/73236a129e4d

https://hg.mozilla.org/mozilla-central/rev/cab512346eee https://hg.mozilla.org/mozilla-central/rev/2c8dacaae28c

I'm going to hold off from landing the rename patch for a bit, because bug 1149524 seems to have started when the WorkerDebuggerGlobalScope.createSandbox patch landed. I find it unlikely that my patch is the cause of this behavior, but its not impossible, so I want khuey or someone else to have a chance to look at it before I land the rest of the patches. Meanwhile, the previous try push for the rename patch looked good overall, but had some fallout from the static analyis failures from the WorkerDebuggerGlobalScope.createSandbox. Since I have to wait anyway, I've made a new try patch to make absolutely sure it will work this time: https://treeherder.mozilla.org/#/jobs?repo=try&revision=988faef1f1f4

Try push for the WorkerDebugger.isFrozen patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=43bfa40974ef

(In reply to Eddy Bruel [:ejpbruel] from comment #156) > Doh, thats because I landed the old patch. Pushing to mozilla-inbound again, > with the correct patch this time: > https://hg.mozilla.org/integration/mozilla-inbound/rev/73236a129e4d Similar issue to comment 149 -- this was missing an 'override' annotation, on GetGlobalJSObject() function-decl (which overrides a nsIGlobalObject function). I pushed "followup 2" to fix that (and I added 'virtual' as well, for consistency w/ the 'virtual...override' function right below this). (If you'd like to catch these things locally, you can build with clang 3.6 and "ac_add_options --enable-warnings-as-errors" in your mozconfig.)

Sorry, I left out the "followup 2" commit URL: https://hg.mozilla.org/integration/mozilla-inbound/rev/f11e199f6e46

Try run for the rename patch shows some infra bustage, but looks good otherwise, so should be good to go. Landing in mozilla-inbound: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=339271da9ef5

https://hg.mozilla.org/mozilla-central/rev/f11e199f6e46 https://hg.mozilla.org/mozilla-central/rev/339271da9ef5

Try push for the WorkerDebugger.isFrozen patch looks good too. Pushing to mozilla-inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/eac9e907082a

\O/

(In reply to Eddy Bruel [:ejpbruel] from comment #167) > \O/ \o/

I'm removing dev-doc-needed from this, as I don't think this particular bug adds any user-visible features to the devtools (it's more like ground work for that feature). If I'm wrong, please feel free to add it back and let me know what the user-visible impact is.

Why is it ok for the "moved" JSClass hook here to not update the pointer stored in the wrapper cache?

In particular, the reason we have an assert that there's a moved hook is so one will get implemented, and having it do nothing is an odd impl. And http://hg.mozilla.org/mozilla-central/file/b2dbee5ca727/dom/base/nsWrapperCache.h#l137 seems to pretty clearly indicate doing nothing is wrong.

(In reply to Boris Zbarsky [:bz] from comment #170) > Why is it ok for the "moved" JSClass hook here to not update the pointer > stored in the wrapper cache? Forgive my ignorance, but I don't know what JSClass hook you are referring to. Could you be more specific?

workerdebuggersandbox_moved

This has been confirmed as an issue, and bug 1260439 has been opened to address it.