1278778 - Show JS stack traces of Fetch requests in Netmonitor

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P2)

Description

[Jarda Snajdr [:jsnajdr]]

This is a followup to bug 1134073. Netmonitor doesn't display stack traces of Fetch requests, because the nsIHttpChannel::asyncOpen2 is not called synchronously when there is still a JS stack, but rather scheduled to a next event loop tick at [1].

[1] https://dxr.mozilla.org/mozilla-central/source/dom/fetch/FetchDriver.cpp#86

Comment 1

[Jarda Snajdr [:jsnajdr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5130d07ad6da

Comment 2

[Jarda Snajdr [:jsnajdr]]

Attachment: Show JS stack traces of Fetch requests in Netmonitor

Fixed by merging FetchDriver::ContinueFetch into FetchDriver::Fetch and removing the async call. Also added a devtools mochitest for the JS stacktrace of a fetch request.

JS stacktraces in devtools start working after this, but a lot of other tests is broken - see the try run. Fetch requests from workers, error handling... My approach seems to be too naive.

Ben, please, do you have any advice?

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8761159 [details] [diff] [review]
Show JS stack traces of Fetch requests in Netmonitor

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

::: dom/fetch/FetchDriver.cpp
@@ -82,5 @@
>  
>    MOZ_RELEASE_ASSERT(!mRequest->IsSynchronous(),
>                       "Synchronous fetch not supported");
>  
> -  return NS_DispatchToCurrentThread(NewRunnableMethod(this, &FetchDriver::ContinueFetch));

Sorry, its harder than this.  See this description of the problem:

https://bugzilla.mozilla.org/show_bug.cgi?id=1206079#c8

Comment 4

[Jarda Snajdr [:jsnajdr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c6dd291730a

Comment 5

[Jarda Snajdr [:jsnajdr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=63a78f71565c

Comment 6

[Jarda Snajdr [:jsnajdr]]

Attachment: Show JS stack traces of Fetch requests in Netmonitor

After reading the instructions in bug 1206079 and getting familiar with the code, I figured out there should be no problem with calling FetchDriverObserver's callbacks synchronously - all they do, after all, is resolving the promise (either directly or through the worker proxy).

The only thing that needed fixing is correctly acquiring and releasing the PromiseWorkerProxy lock. To prevent deadlock, it needs to be already released when calling into FetchDriver::Fetch, because this method can call synchronously into WorkerFetchResolver, which wants to acquire the mutex, too.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8762616 [details] [diff] [review]
Show JS stack traces of Fetch requests in Netmonitor

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

Looks reasonable.  r=me with comment addressed and assuming all tests continue to pass.

::: dom/fetch/Fetch.cpp
@@ +143,4 @@
>      }
> +
> +    // ...but release it before calling Fetch, because mResolver's callback can
> +    // be called synchronously and they want the mutex, too.

Please document that the FetchDriverObserver call backs can be called synchronously or asynchronously in FetchDriver.h.

Comment 8

[Jarda Snajdr [:jsnajdr]]

Attachment: Show JS stack traces of Fetch requests in Netmonitor

Comment 9

[Jarda Snajdr [:jsnajdr]]

Comment on attachment 8762652 [details] [diff] [review]
Show JS stack traces of Fetch requests in Netmonitor

Added a comment about FetchDriverObserver callback sync/async

I ran two try builds with all tests and they seem to be OK. Lots of intermittents, but nothing that looks related to this. I also checked other recent test results on try and they tend to display the same kind of failures - trouble with clipboard etc.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/ae9a292741ef
Show JS stack traces of Fetch requests in Netmonitor r=bkelly

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ae9a292741ef