I consistently get a tab crash when trying to inspect a request object retrieved from the onStateChange callback of a WebProgressListener running in the content process. However I only seem to be able to trigger it from a codepath I am using in a WIP patch...

The stacktrace in DebugMode is:

Hit MOZ_CRASH(If we get here, something is broken) at /builds/worker/checkouts/gecko/netwerk/ipc/DocumentChannel.cpp:367
#01: mozilla::net::DocumentChannel::GetContentCharset(nsTSubstring<char>&)[/Users/juliandescottes/Development/mozilla/hg/mozilla-unified/objdir.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0xad704c]
#02: NS_InvokeByIndex[/Users/juliandescottes/Development/mozilla/hg/mozilla-unified/objdir.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x35339e]
#03: CallMethodHelper::Call()[/Users/juliandescottes/Development/mozilla/hg/mozilla-unified/objdir.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0xe401fc]
#04: XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode)[/Users/juliandescottes/Development/mozilla/hg/mozilla-unified/objdir.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0xe3fdf9]
#05: XPC_WN_GetterSetter(JSContext*, unsigned int, JS::Value*)[/Users/juliandescottes/Development/mozilla/hg/mozilla-unified/objdir.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0xe4232c]
#06: CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&)[/Users/juliandescottes/Development/mozilla/hg/mozilla-unified/objdir.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x65cb5d1]
#07: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason)[/Users/juliandescottes/Development/mozilla/hg/mozilla-unified/objdir.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x65cae51]
#08: js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason)[/Users/juliandescottes/Development/mozilla/hg/mozilla-unified/objdir.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x65cc233]
#09: js::DebuggerObject::call(JSContext*, JS::Handle<js::DebuggerObject*>, JS::Handle<JS::Value>, JS::Handle<JS::GCVector<JS::Value, 0ul, js::TempAllocPolicy> >)[/Users/juliandescottes/Development/mozilla/hg/mozilla-unified/objdir.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x6bf3096]
#10: js::DebuggerObject::CallData::callMethod()[/Users/juliandescottes/Development/mozilla/hg/mozilla-unified/objdir.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x6bf26cd]
#11: bool js::DebuggerObject::CallData::ToNative<&(js::DebuggerObject::CallData::callMethod())>(JSContext*, unsigned int, JS::Value*)[/Users/juliandescottes/Development/mozilla/hg/mozilla-unified/objdir.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x6c02fc1]

The setup is relatively simple, I just setup a progress listener as follows for each and every browsing context:

    webProgress.addProgressListener(
      this,
      Ci.nsIWebProgress.NOTIFY_STATE_DOCUMENT
    );

and the callback:

  onStateChange(progress, request, flag, status) {
    const isStart = flag & Ci.nsIWebProgressListener.STATE_START;
    if (isStart) {
      console.log(request);
    }
  }

In this particular it crashes when we are evaluating the getter contentCharset via _evaluateGetter at https://searchfox.org/mozilla-central/rev/2fc2ccf960c2f7c419262ac7215715c5235948db/devtools/server/actors/object.js#469-483. This makes it very easy to trigger because simply expanding the object triggers the getter evaluation and therefore the crash. If I prevent from automatically resolving getters, then I can still make the tab crash by simply trying to run the getter.

Overall I guess the request object is just unsafe to interact with at that point. Not sure if devtools can do a better job at avoiding this or if this is just an issue with the way the request is implemented.

Hi Valentin,

Trying to gather more information about the specific crash I was facing here.

In a content process, I retrieve a webprogress listener from a top level browsing context using:

browsingContext.docShell
      .QueryInterface(Ci.nsIInterfaceRequestor)
      .getInterface(Ci.nsIWebProgress)

I listen to STATE_DOCUMENT changes:

webProgress.addProgressListener(
      this,
      Ci.nsIWebProgress.NOTIFY_STATE_DOCUMENT
    );

Then in onStateChange I can crash the content page with the following code:

  onStateChange(progress, request, flag, status) {
    const isStart = flag & Ci.nsIWebProgressListener.STATE_START;
    if (isStart) {
      request.QueryInterface(Ci.nsIChannel);
      request.contentCharset;
    }
  }

(see stack trace in the summary)

Is it expected that this triggers a crash or is it something which should be fixed?
(I can provide the patch to trigger this, but the STRs are quite long, let me know)

I don't know if the DocumentChannel is something that we should inspect, which is probably why we're hitting those MOZ_CRASH instances.
Maybe Nika can suggest a different approach here.

(In reply to Valentin Gosu [:valentin] (he/him) from comment #3)

I don't know if the DocumentChannel is something that we should inspect, which is probably why we're hitting those MOZ_CRASH instances.
Maybe Nika can suggest a different approach here.

Thanks!

To be clear it also crashes without DevTools (I imagine that's what you mean by "inspect" here), comment #2 shows code that triggers the crash without inspecting the object.

The tab crashes when DevTools tries to (eagerly) evaluate the contentCharset getter on the DocumentChannel. So you can get the exact same crash without DevTools, simply by using request.QueryInterface(Ci.nsIChannel) & then request.contentCharset. Other nsIChannel getters worked fine, eg originalURI which was the initial reason why I used QueryInterface(Ci.nsIChannel) here.

When developing DocumentChannel, there were many methods on the channel which were never supposed to be called. You can see in the source file that many of these methods are marked as crashing (e.g. contentCharset).

This was quite useful when developing the API as these methods have no reasonable implementation, but specifically in the case of attribute getters, they definitely have the issue that devtools will inspect the properties automatically, potentially leading to crashes. IIRC this is also an issue sometimes with the SHIP SessionHistoryEntry, as some members are content-process-only but exist on nsISHEntry until we can rip out the non-SHIP codepath (e.g. windowState).

An alternative might be to instead warn and return NS_ERROR_NOT_IMPLEMENTED from these methods, but that produces less useful diagnostics during e.g. tests, and is easier to accidentally start using during the development process. Now that we use DocumentChannel and SessionHistoryEntry everywhere though, it might be a change we can make to make the process of inspecting these objects with devtools less risky.

We will probably improve eager evaluation in Bug 1805235, blocking on this.