If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Implement the remainder of the worker debugger API

RESOLVED FIXED

Status

()

Core
DOM: Workers
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: ejpbruel, Assigned: ejpbruel)

Tracking

(Blocks: 3 bugs)

unspecified
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [leave-open])

Attachments

(10 attachments, 37 obsolete attachments)

25.11 KB, patch
khuey
: review+
Details | Diff | Splinter Review
12.67 KB, patch
khuey
: review+
Details | Diff | Splinter Review
15.96 KB, patch
khuey
: review+
Details | Diff | Splinter Review
38.29 KB, patch
khuey
: review+
Details | Diff | Splinter Review
13.63 KB, patch
khuey
: review+
Details | Diff | Splinter Review
11.60 KB, patch
khuey
: review+
Details | Diff | Splinter Review
10.76 KB, patch
khuey
: review+
Details | Diff | Splinter Review
33.90 KB, patch
khuey
: review+
Details | Diff | Splinter Review
14.21 KB, patch
khuey
: review+
Details | Diff | Splinter Review
19.66 KB, patch
khuey
: review+
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
Kyle requested that I opened a new bug for the remainder of the work for bug 757133, so here goes.
(Assignee)

Comment 1

3 years ago
Created attachment 8514902 [details] [diff] [review]
Implement WorkerDebugger.postMessage
Attachment #8514902 - Flags: review?(khuey)
(Assignee)

Comment 2

3 years ago
Created attachment 8514903 [details] [diff] [review]
Implement WorkerDebuggerGlobalScope.reportError
Attachment #8514903 - Flags: review?(khuey)
(Assignee)

Comment 3

3 years ago
Created attachment 8514904 [details] [diff] [review]
Implement WorkerDebuggerGlobalScope.enterEventLoop
Attachment #8514904 - Flags: review?(khuey)
(Assignee)

Comment 4

3 years ago
Created attachment 8514907 [details] [diff] [review]
Implement WorkerDebuggerGlobalScope.reportError

Uploaded the wrong patch.
Attachment #8514903 - Attachment is obsolete: true
Attachment #8514903 - Flags: review?(khuey)
Attachment #8514907 - Flags: review?(khuey)
Blocks: 1003097
Blocks: 893669
Depends on: 1035206
(Assignee)

Comment 5

3 years ago
Review ping
(Assignee)

Updated

3 years ago
Blocks: 1035206
No longer depends on: 1035206
(Assignee)

Updated

3 years ago
(Assignee)

Comment 6

3 years ago
Second review ping.
Comment on attachment 8514902 [details] [diff] [review]
Implement WorkerDebugger.postMessage

New patches are coming.
Attachment #8514902 - Attachment is obsolete: true
Attachment #8514902 - Flags: review?(khuey)
Attachment #8514904 - Attachment is obsolete: true
Attachment #8514904 - Flags: review?(khuey)
Attachment #8514907 - Attachment is obsolete: true
Attachment #8514907 - Flags: review?(khuey)
(Assignee)

Comment 8

3 years ago
Created attachment 8533173 [details] [diff] [review]
Implement WorkerDebugger.postMessage
Attachment #8533173 - Flags: review?(khuey)
(Assignee)

Comment 9

3 years ago
Created attachment 8533174 [details]
Implement WorkerDebuggerGlobalScope.reportError
Attachment #8533174 - Flags: review?(khuey)
(Assignee)

Comment 10

3 years ago
Created attachment 8533175 [details] [diff] [review]
Implement WorkerDebuggerGlobalScope.reportError
Attachment #8533174 - Attachment is obsolete: true
Attachment #8533174 - Flags: review?(khuey)
Attachment #8533175 - Flags: review?(khuey)
(Assignee)

Comment 11

3 years ago
Created attachment 8533177 [details] [diff] [review]
Implement WorkerDebuggerGlobalScope.enterEventLoop

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.
Attachment #8533177 - Flags: review?(khuey)
(Assignee)

Comment 12

3 years ago
Created attachment 8533178 [details] [diff] [review]
Implement WorkerDebuggerGlobalScope.setTimeout

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.
Attachment #8533178 - Flags: review?(khuey)
(Assignee)

Comment 13

3 years ago
Created attachment 8533179 [details] [diff] [review]
Implement WorkerDebuggerGlobalScope.createSandbox

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.
Attachment #8533179 - Flags: review?(khuey)
(Assignee)

Comment 14

3 years ago
Created attachment 8533180 [details] [diff] [review]
Implement WorkerDebuggerGlobalScope.isFrozen

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.
Attachment #8533180 - Flags: review?(khuey)
(Assignee)

Comment 15

3 years ago
Created attachment 8533182 [details] [diff] [review]
Implement Suspend/ResumeWorkersForWindow

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.
Attachment #8533182 - Flags: review?(khuey)
(Assignee)

Comment 16

3 years ago
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)?
Flags: needinfo?(ejpbruel)
(Assignee)

Comment 18

3 years ago
(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.
Flags: needinfo?(ejpbruel)
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?
Attachment #8533175 - Flags: review?(khuey) → review+
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.
Attachment #8533173 - Flags: review?(khuey) → review-
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.
Flags: needinfo?(ejpbruel)
(Assignee)

Comment 23

3 years ago
(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?
Flags: needinfo?(ejpbruel)
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?
(Assignee)

Comment 26

3 years ago
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.
(Assignee)

Comment 27

3 years ago
Created attachment 8544550 [details] [diff] [review]
Implement a WorkerDebuggerGlobalScope.

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.
Attachment #8533173 - Attachment is obsolete: true
Attachment #8533175 - Attachment is obsolete: true
Attachment #8533177 - Attachment is obsolete: true
Attachment #8533178 - Attachment is obsolete: true
Attachment #8533179 - Attachment is obsolete: true
Attachment #8533180 - Attachment is obsolete: true
Attachment #8533182 - Attachment is obsolete: true
Attachment #8533177 - Flags: review?(khuey)
Attachment #8533178 - Flags: review?(khuey)
Attachment #8533179 - Flags: review?(khuey)
Attachment #8533180 - Flags: review?(khuey)
Attachment #8533182 - Flags: review?(khuey)
Attachment #8544550 - Flags: superreview?
Attachment #8544550 - Flags: review?(khuey)
(Assignee)

Comment 28

3 years ago
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.
Attachment #8544550 - Flags: superreview?
(Assignee)

Comment 29

3 years ago
Created attachment 8544554 [details] [diff] [review]
Implement worker debugger runnables

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.
Attachment #8544554 - Flags: review?(khuey)
(Assignee)

Comment 30

3 years ago
Created attachment 8544556 [details] [diff] [review]
Implement WorkerDebugger.initialize

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.
Attachment #8544556 - Flags: review?(khuey)
(Assignee)

Comment 31

3 years ago
Created attachment 8544557 [details] [diff] [review]
Implement WorkerDebugger.initialize

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.
Attachment #8544557 - Flags: review?(khuey)
(Assignee)

Updated

3 years ago
Attachment #8544557 - Attachment is obsolete: true
Attachment #8544557 - Flags: review?(khuey)
(Assignee)

Comment 32

3 years ago
Created attachment 8544559 [details] [diff] [review]
Implement WorkerDebugger.postMessage

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.
Attachment #8544559 - Flags: review?(khuey)
(Assignee)

Comment 33

3 years ago
Created attachment 8544560 [details] [diff] [review]
Implement WorkerDebuggerGlobalScope.reportError

This patch is essentially the same as before.
Attachment #8544560 - Flags: review?(khuey)
(Assignee)

Comment 34

3 years ago
Created attachment 8544572 [details] [diff] [review]
Implement WorkerDebuggerGlobalScope.enterEventLoop

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.
Attachment #8544572 - Flags: review?(khuey)
(Assignee)

Comment 35

3 years ago
Created attachment 8544573 [details] [diff] [review]
Implement WorkerDebuggerGlobalScope.setTimeout
Attachment #8544573 - Flags: review?(khuey)
(Assignee)

Comment 36

3 years ago
Created attachment 8544574 [details] [diff] [review]
Implement WorkerDebuggerGlobalScope.createSandbox
Attachment #8544574 - Flags: review?(khuey)
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.
Attachment #8544550 - Flags: review?(khuey) → review-
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.
Attachment #8544554 - Flags: review?(khuey) → review-
And, fwiw, having the correct patch for the WorkerDebuggerGlobalScope would be useful because it would allow me to apply the rest of them.
Flags: needinfo?(ejpbruel)
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?
Attachment #8544556 - Flags: review?(khuey) → review-
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.
Attachment #8544559 - Flags: review?(khuey) → review-
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?
Attachment #8544560 - Flags: review?(khuey) → review-
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?
Attachment #8544572 - Flags: review?(khuey)
(Assignee)

Comment 44

3 years ago
Created attachment 8548881 [details] [diff] [review]
Implement a WorkerDebuggerGlobalScope.

Correct patch this time.
Attachment #8544550 - Attachment is obsolete: true
Flags: needinfo?(ejpbruel)
Attachment #8548881 - Flags: review?(khuey)
(Assignee)

Comment 45

3 years ago
(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*.
(Assignee)

Comment 46

3 years ago
Created attachment 8548903 [details] [diff] [review]
Implement worker debugger runnables

See comment 45 for a response to your review comments.
Assignee: nobody → ejpbruel
Attachment #8544554 - Attachment is obsolete: true
Attachment #8548903 - Flags: review?(khuey)
(Assignee)

Comment 47

3 years ago
Created attachment 8548963 [details] [diff] [review]
Implement WorkerDebugger.initialize

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).
Attachment #8544556 - Attachment is obsolete: true
Attachment #8548963 - Flags: review?(khuey)
(Assignee)

Comment 48

3 years ago
Created attachment 8548976 [details] [diff] [review]
Implement WorkerDebugger.postMessage

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.
Attachment #8544559 - Attachment is obsolete: true
Attachment #8548976 - Flags: review?(khuey)
(Assignee)

Comment 49

3 years ago
(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?
Flags: needinfo?(khuey)
(Assignee)

Comment 50

3 years ago
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()?
Attachment #8548963 - Flags: review?(khuey) → review-
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.
Attachment #8548976 - Flags: review?(khuey) → review-
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.
Attachment #8548881 - Flags: review?(khuey) → review-
Comment on attachment 8548903 [details] [diff] [review]
Implement worker debugger runnables

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

r=me
Attachment #8548903 - Flags: review?(khuey) → review+
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.
Flags: needinfo?(khuey)
(Assignee)

Comment 57

3 years ago
Created attachment 8564218 [details] [diff] [review]
Implement a WorkerDebuggerGlobalScope

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.
Attachment #8544560 - Attachment is obsolete: true
Attachment #8544572 - Attachment is obsolete: true
Attachment #8544573 - Attachment is obsolete: true
Attachment #8544574 - Attachment is obsolete: true
Attachment #8548881 - Attachment is obsolete: true
Attachment #8544573 - Flags: review?(khuey)
Attachment #8544574 - Flags: review?(khuey)
Attachment #8564218 - Flags: review?(khuey)
(Assignee)

Updated

3 years ago
Attachment #8564218 - Attachment description: patch → Implement a WorkerDebuggerGlobalScope
Keywords: dev-doc-needed
(Assignee)

Comment 58

3 years ago
Created attachment 8564996 [details] [diff] [review]
Implement worker debugger runnables

Moved the implementation of WorkerDebuggerRunnable from the WorkerDebugger.postMessage patch to this one, as you requested.
Attachment #8548903 - Attachment is obsolete: true
Attachment #8564996 - Flags: review?(khuey)
(Assignee)

Comment 59

3 years ago
Created attachment 8564998 [details] [diff] [review]
Implement WorkerDebugger.initialize

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).
Attachment #8548963 - Attachment is obsolete: true
Attachment #8564998 - Flags: review?(khuey)
(Assignee)

Updated

3 years ago
Attachment #8564998 - Attachment description: patch → Implement WorkerDebugger.initialize
(Assignee)

Comment 60

3 years ago
Created attachment 8564999 [details] [diff] [review]
Implement WorkerDebugger.postMessage

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.
Attachment #8548976 - Attachment is obsolete: true
Attachment #8564999 - Flags: review?(khuey)
Attachment #8564218 - Flags: review?(khuey) → review+
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.
Attachment #8564996 - Flags: review?(khuey) → review+
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.
Attachment #8564998 - Flags: review?(khuey) → review-
Attachment #8564999 - Flags: review?(khuey)
Component: Developer Tools: Debugger → DOM: Workers
Product: Firefox → Core
(Assignee)

Comment 64

3 years ago
Created attachment 8566132 [details] [diff] [review]
Implement worker debugger runnables

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.
Attachment #8564996 - Attachment is obsolete: true
Attachment #8566132 - Flags: review?(khuey)
(Assignee)

Comment 65

3 years ago
Created attachment 8566134 [details] [diff] [review]
Implement WorkerDebugger.initialize

This patch compiles on my machine. Tests seem to be passing as well.
Attachment #8564998 - Attachment is obsolete: true
Attachment #8566134 - Flags: review?(khuey)
(Assignee)

Comment 66

3 years ago
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.
(Assignee)

Comment 67

3 years ago
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?
Flags: needinfo?(khuey)
Attachment #8566132 - Flags: review?(khuey) → review+
(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.
Flags: needinfo?(khuey)
(Assignee)

Comment 69

3 years ago
Try push for the WorkerDebuggerGlobalScope patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=90c6b39a0617
(Assignee)

Comment 70

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

Comment 71

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

Comment 72

3 years ago
(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?
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 73

3 years ago
Created attachment 8567031 [details] [diff] [review]
Implement WorkerDebugger.initialize

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.
Attachment #8566134 - Attachment is obsolete: true
Attachment #8566134 - Flags: review?(khuey)
Attachment #8567031 - Flags: review?(khuey)
(Assignee)

Comment 74

3 years ago
Created attachment 8567034 [details] [diff] [review]
Implement WorkerDebugger.postMessage

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.
Attachment #8564999 - Attachment is obsolete: true
Attachment #8567034 - Flags: review?(khuey)
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.
Flags: needinfo?(bzbarsky)
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.
(Assignee)

Comment 77

3 years ago
(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.
Flags: needinfo?(bzbarsky)
> 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...
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 79

3 years ago
Created attachment 8567964 [details] [diff] [review]
patch

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.
Attachment #8567964 - Flags: feedback?(bzbarsky)
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).
Attachment #8567964 - Flags: feedback?(bzbarsky) → feedback-
(Assignee)

Comment 81

3 years ago
Created attachment 8567979 [details] [diff] [review]
patch

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.
Attachment #8567964 - Attachment is obsolete: true
Attachment #8567979 - Flags: feedback?(bzbarsky)
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.
Attachment #8567979 - Flags: feedback?(bzbarsky) → feedback+
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.
(Assignee)

Comment 85

3 years ago
Created attachment 8569831 [details] [diff] [review]
WorkerDebuggerGlobalScope

Here is an updated patch that implements #1 from comment 75.
Attachment #8564218 - Attachment is obsolete: true
Attachment #8567979 - Attachment is obsolete: true
Attachment #8569831 - Flags: review?(khuey)
(Assignee)

Comment 86

3 years ago
Created attachment 8569832 [details] [diff] [review]
worker-debugger-runnables

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.
Attachment #8566132 - Attachment is obsolete: true
Attachment #8569832 - Flags: review?(khuey)
(Assignee)

Comment 87

3 years ago
Created attachment 8569833 [details] [diff] [review]
WorkerDebugger.initialize
Attachment #8567031 - Attachment is obsolete: true
Attachment #8567031 - Flags: review?(khuey)
Attachment #8569833 - Flags: review?(khuey)
(Assignee)

Comment 88

3 years ago
Created attachment 8569834 [details] [diff] [review]
WorkerDebugger.postMessage
Attachment #8567034 - Attachment is obsolete: true
Attachment #8567034 - Flags: review?(khuey)
Attachment #8569834 - Flags: review?(khuey)
(Assignee)

Comment 89

3 years ago
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.
(Assignee)

Comment 91

3 years ago
(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
Attachment #8569831 - Flags: review?(khuey) → review+
Attachment #8569832 - Flags: review?(khuey) → review+
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
Attachment #8569833 - Flags: review?(khuey) → review-
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
Attachment #8569834 - Flags: review?(khuey) → review+
(Assignee)

Comment 95

3 years ago
(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?
Flags: needinfo?(khuey)
(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.
Flags: needinfo?(khuey)
(Assignee)

Comment 97

3 years ago
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
Whiteboard: [leave-open]
(Assignee)

Comment 98

3 years ago
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
(Assignee)

Comment 100

3 years ago
(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?
Flags: needinfo?(khuey)
See comment 96?  Alternatively push it to try?
Flags: needinfo?(khuey)
(Assignee)

Comment 102

3 years ago
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.
(Assignee)

Comment 103

3 years ago
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?
Flags: needinfo?(khuey)
ejpbruel is going to schedule something.
Flags: needinfo?(khuey)
(Assignee)

Comment 105

3 years ago
Created attachment 8575987 [details] [diff] [review]
WorkerDebugger.initialize

New patch with comments and leak addressed, as discussed on irc.
Attachment #8569833 - Attachment is obsolete: true
Attachment #8575987 - Flags: review?(khuey)
(Assignee)

Comment 106

3 years ago
Created attachment 8575990 [details] [diff] [review]
WorkerDebuggerGlobalScope.reportError

Five more patches to go after this one.
Attachment #8575990 - Flags: review?(khuey)

Comment 107

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

Comment 108

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

Comment 109

3 years ago
Created attachment 8576041 [details] [diff] [review]
WorkerDebuggerGlobalScope.enterEventLoop

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?
Attachment #8576041 - Flags: review?(khuey)
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.
Attachment #8575987 - Flags: review?(khuey) → review+
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.
Attachment #8575990 - Flags: review?(khuey) → review+
(Assignee)

Comment 114

3 years ago
Created attachment 8577268 [details] [diff] [review]
WorkerDebuggerGlobalScope.createSandbox

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.
Attachment #8577268 - Flags: review?(khuey)
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.
Attachment #8576041 - Flags: review?(khuey) → review+
(Assignee)

Comment 116

3 years ago
(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.
Flags: needinfo?(jimb)

Comment 117

3 years ago
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.)
Flags: needinfo?(jimb)
(Assignee)

Comment 118

3 years ago
(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?
(Assignee)

Comment 119

3 years ago
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
(Assignee)

Comment 120

3 years ago
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?
Flags: needinfo?(khuey)
(Assignee)

Comment 121

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

Comment 122

3 years ago
Created attachment 8578553 [details] [diff] [review]
WorkerDebuggerGlobalScope.setImmediate

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.
Attachment #8578553 - Flags: review?(khuey)
The build failure on Windows is probably what I mentioned in comment 94.
Flags: needinfo?(khuey)
https://hg.mozilla.org/mozilla-central/rev/757de8e339e0
Flags: in-testsuite+

Comment 125

3 years ago
(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.
Attachment #8578553 - Flags: review?(khuey) → review+
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.
Attachment #8577268 - Flags: review?(khuey)
(Assignee)

Comment 127

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

Comment 128

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

Comment 129

3 years ago
Finally, try push looks green. Pushing to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a5061645ba1
https://hg.mozilla.org/mozilla-central/rev/7a5061645ba1
(Assignee)

Comment 131

3 years ago
Created attachment 8581798 [details] [diff] [review]
Rename Suspend/Resume to Freeze/Thaw

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).
Attachment #8581798 - Flags: review?(khuey)
(Assignee)

Comment 132

3 years ago
Created attachment 8581803 [details] [diff] [review]
WorkerDebugger.isFrozen

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.
Attachment #8581803 - Flags: review?(khuey)
(Assignee)

Comment 133

3 years ago
Try run for the WorkerDebuggerGlobalScope.reportError patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c35825c0702a
(Assignee)

Comment 134

3 years ago
Try run looks good, pushing to mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f837658dea2
(Assignee)

Comment 135

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

Comment 136

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

Comment 137

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

Comment 138

3 years ago
Created attachment 8584342 [details] [diff] [review]
WorkerDebuggerGlobalScope.createSandbox

New patch for WorkerDebuggerGlobalScope.createSandbox, with comments addressed.
Attachment #8577268 - Attachment is obsolete: true
Attachment #8584342 - Flags: review?(khuey)
https://hg.mozilla.org/mozilla-central/rev/6f837658dea2
https://hg.mozilla.org/mozilla-central/rev/3b30137b0f6a
Attachment #8584342 - Flags: review?(khuey) → review+
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.
Attachment #8581798 - Flags: review?(khuey) → review+
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?
(Assignee)

Comment 142

3 years ago
(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.
(Assignee)

Comment 143

3 years ago
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.
Attachment #8581803 - Flags: review?(khuey) → review+
(Assignee)

Comment 145

3 years ago
(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.
(Assignee)

Comment 146

3 years ago
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
(Assignee)

Comment 147

3 years ago
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
(Assignee)

Comment 148

3 years ago
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
(Assignee)

Comment 151

3 years ago
(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!
(Assignee)

Comment 152

3 years ago
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
(Assignee)

Comment 153

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

Comment 154

3 years ago
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
(Assignee)

Comment 156

3 years ago
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
(Assignee)

Comment 158

3 years ago
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
(Assignee)

Comment 159

3 years ago
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
https://hg.mozilla.org/mozilla-central/rev/73236a129e4d
(Assignee)

Comment 163

3 years ago
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
(Assignee)

Comment 165

3 years ago
Try push for the WorkerDebugger.isFrozen patch looks good too. Pushing to mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eac9e907082a
https://hg.mozilla.org/mozilla-central/rev/eac9e907082a
(Assignee)

Comment 167

3 years ago
\O/
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(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.
Keywords: dev-doc-needed
Why is it ok for the "moved" JSClass hook here to not update the pointer stored in the wrapper cache?
Flags: needinfo?(ejpbruel)
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.
(Assignee)

Comment 172

2 years ago
(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?
Flags: needinfo?(ejpbruel) → needinfo?(bzbarsky)
workerdebuggersandbox_moved
Flags: needinfo?(bzbarsky) → needinfo?(ejpbruel)
Depends on: 1260439
(Assignee)

Comment 174

2 years ago
This has been confirmed as an issue, and bug 1260439 has been opened to address it.
Flags: needinfo?(ejpbruel)
You need to log in before you can comment on or make changes to this bug.