Closed Bug 1278778 Opened 5 years ago Closed 5 years ago

Show JS stack traces of Fetch requests in Netmonitor

Categories

(DevTools :: Netmonitor, defect, P2)

defect

Tracking

(firefox50 fixed)

RESOLVED FIXED
Firefox 50
Tracking Status
firefox50 --- fixed

People

(Reporter: jsnajdr, Assigned: jsnajdr)

References

Details

Attachments

(1 file, 2 obsolete files)

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
See Also: → 1269765
Assignee: nobody → jsnajdr
Blocks: 1134073
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?
Attachment #8761159 - Flags: feedback?(bkelly)
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
Attachment #8761159 - Flags: feedback?(bkelly) → feedback-
Depends on: 1206079
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.
Attachment #8762616 - Flags: review?(bkelly)
Attachment #8761159 - Attachment is obsolete: true
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.
Attachment #8762616 - Flags: review?(bkelly) → review+
Attachment #8762616 - Attachment is obsolete: true
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.
Attachment #8762652 - Flags: review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/ae9a292741ef
Show JS stack traces of Fetch requests in Netmonitor r=bkelly
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ae9a292741ef
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Duplicate of this bug: 1206079
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.