1535071 - Tab crash when reporting exception in worker after refreshing page a ton

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Logan Smyth [:loganfsmyth]]

Attachment: What it should look like just before you refresh over and over

There may be easier repros for this, but this is how I came across this. Unfortunately it is a few steps. Hopefully the stack trace is helpful if reproducing is too hard.

1. cd devtools/client/debugger/new
2. yarn - installs all Nodejs dependencies, you may also need to install yarn
3. yarn start - start a development server for a the debugger
4. Open a copy of Nightly
5. Enable devtools.debugger.remote-enabled and set devtools.debugger.remote-port to 6080
6. Open http://localhost:8000/integration/examples/doc-sourcemaps.html in Nightly, as an example. The specific site probably doesn't matter.
7. Open a new terminal and run ./mach run
8. Open http://localhost:8000/?react_perf&firefox-tab=child1/frameTarget1 to start a copy of the debugger hosted as a standalone page. They may take a minute to load the first time.
9. Open the devtools in MC's firefox copy, so you are debugging the copy of the debugger hosted on localhost:8000
10. In the devtools bottom pane at the bottom, ensure the Debugger is open/focused and press Cmd+P and type "Editor/Tabs.js" to open this file for debugging. I've attached a screenshot of what this looks like to make sure we're on the same page.
11. Refresh the "localhost:8000/?react_perf&firefox-tab=child1/frameTarget1"-hosted copy of the debugger, and keep refreshing it every time the "Editor/Tabs.js" file starts loading in the debugger pane.

AR:
Eventually, after maybe 10-30ish refreshes, the tab will crash

ER:
No crashes

I attached my debugger and LLDB shows this:

* thread #37, name = 'DOM Worker', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x000000010a72bd54 XUL`JS_IsGlobalObject(JSObject*) [inlined] JSObject::getClass(this=0x0000000000000000) const at JSObject.h:113 [opt]
    frame #1: 0x000000010a72bd54 XUL`JS_IsGlobalObject(JSObject*) [inlined] bool JSObject::is<js::GlobalObject>(this=0x0000000000000000) const at GlobalObject.h:981 [opt]
    frame #2: 0x000000010a72bd54 XUL`JS_IsGlobalObject(obj=0x0000000000000000) at jsapi.cpp:1061 [opt]
    frame #3: 0x0000000108600a02 XUL`mozilla::dom::AutoJSAPI::ReportException(this=0x00007000030bc510) at ScriptSettings.cpp:488 [opt]
    frame #4: 0x00000001083f333b XUL`mozilla::dom::WorkerRunnable::Run(this=0x00000001878a6060) at WorkerRunnable.cpp:365 [opt]
    frame #5: 0x00000001083e3d17 XUL`mozilla::dom::WorkerPrivate::DoRunLoop(this=0x0000000101447800, aCx=0x0000000184b51000) at WorkerPrivate.cpp:2727 [opt]
    frame #6: 0x00000001083c37e1 XUL`mozilla::dom::workerinternals::(anonymous namespace)::WorkerThreadPrimaryRunnable::Run(this=<unavailable>) at RuntimeService.cpp:2293 [opt]
    frame #7: 0x0000000104d1c887 XUL`nsThread::ProcessNextEvent(this=0x000000018458bde0, aMayWait=<unavailable>, aResult=0x00007000030bcdcf) at nsThread.cpp:1179 [opt]
    frame #8: 0x0000000104d207e8 XUL`NS_ProcessNextEvent(aThread=<unavailable>, aMayWait=true) at nsThreadUtils.cpp:482 [opt]
    frame #9: 0x0000000105404d22 XUL`mozilla::ipc::MessagePumpForNonMainThreads::Run(this=0x000000018459aa80, aDelegate=0x000000018458c120) at MessagePump.cpp:333 [opt]
    frame #10: 0x00000001053a04e5 XUL`MessageLoop::Run() [inlined] MessageLoop::RunHandler(this=<unavailable>) at message_loop.cc:308 [opt]
    frame #11: 0x00000001053a04e0 XUL`MessageLoop::Run(this=<unavailable>) at message_loop.cc:290 [opt]
    frame #12: 0x0000000104d18f95 XUL`nsThread::ThreadFunc(aArg=<unavailable>) at nsThread.cpp:454 [opt]
    frame #13: 0x0000000100fa7430 libnss3.dylib`_pt_root(arg=0x0000000181e34b30) at ptthread.c:201 [opt]
    frame #14: 0x00007fff7ef38661 libsystem_pthread.dylib`_pthread_body + 340
    frame #15: 0x00007fff7ef3850d libsystem_pthread.dylib`_pthread_start + 377
    frame #16: 0x00007fff7ef37bf9 libsystem_pthread.dylib`thread_start + 13

where we are ending up passing a nullptr into JS_IsGlobalObject.

Comment 1

[Logan Smyth [:loganfsmyth]]

Tagging Brian in on this since I think this is either a new bug, or quite possibly an old bug that was only exposed by https://bugzilla.mozilla.org/show_bug.cgi?id=1527203. Either way he's probably a good person to do more exploration.

From a quick bit of searching, my theory is that we are killing the debugger server before setDebuggerReady is called, meaning that the content script hasn't run yet and thus the worker debuggee global does not exist yet. Maybe that could have happened before too, but now it is at least much easier to create and kill the server before the debuggee script is ready.

It looks like this code for reporting exceptions will try to use the debuggee global for the exception context even if the exception was triggered by debugger-server code, so maybe that is a core issue? It seems like the exception global should be whichever global caused the exception in the first place.

Comment 2

[Brian Hackett [Laid off!]]

(In reply to Logan Smyth [:loganfsmyth] from comment #1)

Tagging Brian in on this since I think this is either a new bug, or quite possibly an old bug that was only exposed by https://bugzilla.mozilla.org/show_bug.cgi?id=1527203. Either way he's probably a good person to do more exploration.

From a quick bit of searching, my theory is that we are killing the debugger server before setDebuggerReady is called, meaning that the content script hasn't run yet and thus the worker debuggee global does not exist yet. Maybe that could have happened before too, but now it is at least much easier to create and kill the server before the debuggee script is ready.

It looks like this code for reporting exceptions will try to use the debuggee global for the exception context even if the exception was triggered by debugger-server code, so maybe that is a core issue? It seems like the exception global should be whichever global caused the exception in the first place.

Thanks for the detailed STR, I'm able to reproduce the crash here. I think this is a regression from bug 1527203, and that the error is being reported from debugger code that ran before the attach finished. With the fix I'm about to post (bugzilla+phabricator isn't a very comment-friendly workflow), the error reported afterwards is below:

JavaScript error: resource://devtools/shared/protocol.js, line 1299: Error: Connection closed, pending request to server1.conn0.child1/workerTarget56, type connect failed

Comment 3

[Brian Hackett [Laid off!]]

Attachment: Bug 1535071 - Use the debugger global scope for worker error reporting if necessary, r=smaug.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

(yes, bugzilla+phabricator isn't comment friendly workflow at all)

baku should review this too. I think he has reviewed many of the worker+debugger patches.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6ec2f3eed8dc
Use the debugger global scope for worker error reporting if necessary, r=smaug,baku.

Comment 6

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/6ec2f3eed8dc

Comment 7

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/projects/ash/rev/6ec2f3eed8dcfc418c75fded9464338b561e15f6
Bug 1535071 - Use the debugger global scope for worker error reporting if necessary, r=smaug,baku.

Comment 8

[Julien Cristau [:jcristau]]

Is this worth fixing in 67? Please request uplift to beta if so.

Comment 9

[Logan Smyth [:loganfsmyth]]

This was uplifted as one of the patches in https://bugzilla.mozilla.org/show_bug.cgi?id=1539817

Comment 10

[Julien Cristau [:jcristau]]

sigh ok...