Closed Bug 1326298 Opened 3 years ago Closed 2 years ago

Support off-main-thread delivery with start/stop/error listeners

Categories

(WebExtensions :: Request Handling, defect, P3)

53 Branch
defect

Tracking

(firefox53 wontfix, firefox54 ?, firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox53 --- wontfix
firefox54 --- ?
firefox55 --- fixed
Blocking Flags:
webextensions +

People

(Reporter: kmag, Assigned: mixedpuppy)

References

(Blocks 1 open bug)

Details

(Whiteboard: triaged)

Attachments

(1 file)

We currently attach request listeners to every request whenever any onStart, onStop, or onError listener is present, which breaks off-main-thread delivery to the HTML 5 parser. There's no technical reason this is necessary, so we should fix it.

The simplest solution would be to create a native helper which forwards onStartRequest and onStopRequests to our JS listener, and forwards onDataAvailable directly to the wrapped listener.

We should also simply avoid attaching these listeners when no extant listeners have filters which match the request.
Blocks: 1326299
kris/shane transfering info
Assignee: kmaglione+bmo → mixedpuppy
Flags: needinfo?(kmaglione+bmo)
Priority: -- → P3
Whiteboard: triaged
Kris, Just a couple initial questions since I'm not familiar with where/what the boundary is for the html5 parser.

Is the problem listener the nsIRequestObserver/nsIStreamListener?  At what point (or what causes) is it determined to run the request on the main thread?  Is it simply having a js listener?

By "native helper" do you mean a C++ level listener?

Assuming the above results in the need for a C++ listener, Can we call into js to determine if the request matches any listeners, or would that also result in the request running on the main thread?
The basic problem is this:

In order to support off-main-thread delivery of request data, every listener in the listener chain needs to be implemented in native code, and to be thread-safe. Before delivery can be redirected to a background thread, the channel's listener needs to be queried to nsIThreadRetargetableStreamListener, and the entire listener chain checked for thread safety.

We handle start and stop listeners by querying channels to nsITraceableChannel and installing our own JS-implemented listener, which breaks the thread safety, and disables retargeting. The good news is that only the onDataAvailable method of the listener needs to be called in a background thread, and we don't actually need to deal with those callbacks. We just forward them to the next listener in the chain. So we need a native helper which can wrap our JS listener, and call its onStartRequest and onStopRequest listeners, but just forward onDataAvailable and checkListenerChain to the next handler in the chain.

Later on down the line, when we support responseData filtering (bug 1255894), we'll also need a thread-safe way to forward that data to listeners in add-on processes. We don't need to worry about that quite yet, but it would be nice to keep it in mind as a future use case when designing the interface.
Flags: needinfo?(kmaglione+bmo)
webextensions: --- → +
Comment on attachment 8850158 [details]
Bug 1326298 implement off-main-thread delivery with start/stop/error listeners,

https://reviewboard.mozilla.org/r/122850/#review125070

::: toolkit/modules/addons/WebRequest.jsm:33
(Diff revision 1)
>  XPCOMUtils.defineLazyModuleGetter(this, "WebRequestUpload",
>                                    "resource://gre/modules/WebRequestUpload.jsm");
>  
>  XPCOMUtils.defineLazyGetter(this, "ExtensionError", () => ExtensionUtils.ExtensionError);
>  
> +XPCOMUtils.defineLazyGetter(this, "WebExtensionListener", () =>

No need for a lazy getter.

::: toolkit/modules/addons/nsWebRequestListener.h:34
(Diff revision 1)
> +  nsCOMPtr<nsIStreamListener> m_origStreamListener;
> +  nsCOMPtr<nsIStreamListener> m_targetStreamListener;
> +  nsCOMPtr<nsISupports>       m_context;

mOrigStreamListener;
    ...

::: toolkit/modules/addons/nsWebRequestListener.h:36
(Diff revision 1)
> +
> +private:
> +  ~nsWebRequestListener() {}
> +  nsCOMPtr<nsIStreamListener> m_origStreamListener;
> +  nsCOMPtr<nsIStreamListener> m_targetStreamListener;
> +  nsCOMPtr<nsISupports>       m_context;

No need to store this.

::: toolkit/modules/addons/nsWebRequestListener.cpp:7
(Diff revision 1)
> +#ifdef DEBUG
> +#include "MainThreadUtils.h"
> +#endif

Please remove.

::: toolkit/modules/addons/nsWebRequestListener.cpp:12
(Diff revision 1)
> +#ifdef DEBUG
> +#include "MainThreadUtils.h"
> +#endif
> +
> +namespace mozilla {
> +namespace net {

s/net/extensions/

::: toolkit/modules/addons/nsWebRequestListener.cpp:22
(Diff revision 1)
> +                  nsIRequestObserver,
> +                  nsIThreadRetargetableStreamListener)
> +
> +/* void init (in nsIStreamListener aStreamListener, in nsITraceableChannel aTraceableChannel); */
> +NS_IMETHODIMP
> +nsWebRequestListener::Init(nsIStreamListener *aStreamListener, nsITraceableChannel *aTraceableChannel, nsISupports *aContext)

No context, please.

::: toolkit/modules/addons/nsWebRequestListener.cpp:35
(Diff revision 1)
> +NS_IMETHODIMP
> +nsWebRequestListener::OnStartRequest(nsIRequest *request, nsISupports * aCtxt)
> +{
> +  nsresult rv = NS_OK;
> +
> +  if (m_targetStreamListener)

No ifs for either of these. Please just assert.

::: toolkit/modules/addons/nsWebRequestListener.cpp:36
(Diff revision 1)
> +nsWebRequestListener::OnStartRequest(nsIRequest *request, nsISupports * aCtxt)
> +{
> +  nsresult rv = NS_OK;
> +
> +  if (m_targetStreamListener)
> +    rv = m_targetStreamListener->OnStartRequest(request, m_context);

`NS_ENSURE_SUCCESS(rv, rv)` after each call. Same for the others.

::: toolkit/modules/addons/nsWebRequestListener.cpp:50
(Diff revision 1)
> +nsWebRequestListener::OnStopRequest(nsIRequest *request, nsISupports *aCtxt,
> +                                           nsresult aStatus)
> +{
> +  nsresult rv = NS_OK;
> +
> +  if (m_origStreamListener)

Same here.

::: toolkit/modules/addons/nsWebRequestListener.cpp:76
(Diff revision 1)
> +
> +NS_IMETHODIMP
> +nsWebRequestListener::CheckListenerChain()
> +{
> +    NS_ASSERTION(NS_IsMainThread(), "Should be on main thread!");
> +    nsresult rv = NS_OK;

Unnecessary.

::: toolkit/modules/addons/nsWebRequestListener.cpp:80
(Diff revision 1)
> +    NS_ASSERTION(NS_IsMainThread(), "Should be on main thread!");
> +    nsresult rv = NS_OK;
> +    nsCOMPtr<nsIThreadRetargetableStreamListener> retargetableListener =
> +        do_QueryInterface(m_origStreamListener, &rv);
> +    if (retargetableListener) {
> +        rv = retargetableListener->CheckListenerChain();

return ...->CheckListenerChain();

::: toolkit/modules/addons/nsWebRequestListener.cpp:82
(Diff revision 1)
> +    nsCOMPtr<nsIThreadRetargetableStreamListener> retargetableListener =
> +        do_QueryInterface(m_origStreamListener, &rv);
> +    if (retargetableListener) {
> +        rv = retargetableListener->CheckListenerChain();
> +    }
> +    return rv;

return NS_OK;

::: toolkit/modules/addons/nsWebRequestListener.cpp:90
(Diff revision 1)
> +NS_GENERIC_FACTORY_CONSTRUCTOR(nsWebRequestListener)
> +
> +NS_DEFINE_NAMED_CID(NS_WEBREQUESTLISTENER_CID);
> +
> +static const mozilla::Module::CIDEntry kWebRequestListenerCIDs[] = {
> +    { &kNS_WEBREQUESTLISTENER_CID, false, nullptr, nsWebRequestListenerConstructor },
> +    { nullptr }
> +};
> +
> +static const mozilla::Module::ContractIDEntry kWebRequestListenerContracts[] = {
> +    { NS_WEBREQUESTLISTENER_CONTRACTID, &kNS_WEBREQUESTLISTENER_CID },
> +    { nullptr }
> +};
> +
> +static const mozilla::Module kWebRequestListenerModule = {
> +    mozilla::Module::kVersion,
> +    kWebRequestListenerCIDs,
> +    kWebRequestListenerContracts
> +};
> +
> +NSMODULE_DEFN(nsWebRequestListenerModule) = &kWebRequestListenerModule;

Please move this to nsToolkitCompsModule.cpp
Comment on attachment 8850158 [details]
Bug 1326298 implement off-main-thread delivery with start/stop/error listeners,

https://reviewboard.mozilla.org/r/122850/#review125622

Looks good. Thanks!

::: toolkit/modules/addons/WebRequest.jsm:331
(Diff revision 3)
> -  this.orig = null;
> +  let WebExtensionListener = Components.Constructor("@mozilla.org/webextensions/webRequestListener;1",
> +                                                    "nsIWebRequestListener", "init");

Please do this at the top-level.

::: toolkit/modules/addons/moz.build:11
(Diff revision 3)
> +
> +
> +XPIDL_SOURCES += [
> +    'nsIWebRequestListener.idl',
> +]
> +

Nit: Remove empty line.

::: toolkit/modules/addons/nsIWebRequestListener.idl:19
(Diff revision 3)
> +            in nsITraceableChannel aTraceableChannel);
> +};
> +
> +%{C++
> +#define NS_WEBREQUESTLISTENER_CID                    \
> + { /* ebea9901-e135-b546-82e2-052666992dbb */       \

Nit: Please move comment outside of the macro body.

::: toolkit/modules/addons/nsWebRequestListener.h:21
(Diff revision 3)
> +class nsWebRequestListener final : public nsIWebRequestListener
> +                                 , public nsIStreamListener
> +                                 , public nsIThreadRetargetableStreamListener
> +{
> +public:
> +  NS_DECL_ISUPPORTS

Hm... We should probably implement cycle collection for this, in case some JS consumer keeps a reference to it. But channels aren't cycle collected, so for the current use it's not entirely necessary.

Please add a comment to the IDL explaining that it's not cycle collected, and the implications of that.

::: toolkit/modules/addons/nsWebRequestListener.cpp:16
(Diff revision 3)
> +                  nsIWebRequestListener,
> +                  nsIStreamListener,
> +                  nsIRequestObserver,
> +                  nsIThreadRetargetableStreamListener)
> +
> +/* void init (in nsIStreamListener aStreamListener, in nsITraceableChannel aTraceableChannel); */

Not necessary.

::: toolkit/modules/addons/nsWebRequestListener.cpp:20
(Diff revision 3)
> +  NS_ASSERTION(aStreamListener, "Should have aStreamListener");
> +  NS_ASSERTION(aTraceableChannel, "Should have aTraceableChannel");

s/NS_ASSERTION/MOZ_ASSERT/

Same elsewhere.

::: toolkit/modules/addons/nsWebRequestListener.cpp:23
(Diff revision 3)
> +  aTraceableChannel->SetNewListener(this, getter_AddRefs(mOrigStreamListener));
> +  return NS_OK;

return aTraceableChannel->SetNewListener(...);

::: toolkit/modules/addons/nsWebRequestListener.cpp:34
(Diff revision 3)
> +{
> +  NS_ASSERTION(mTargetStreamListener, "Should have mTargetStreamListener");
> +  nsresult rv = mTargetStreamListener->OnStartRequest(request, aCtxt);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  NS_ASSERTION(mOrigStreamListener, "Should have mOrigStreamListener");

Please move both assertions to the top of the method.

::: toolkit/modules/addons/nsWebRequestListener.cpp:35
(Diff revision 3)
> +  rv = mOrigStreamListener->OnStartRequest(request, aCtxt);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  return rv;

Nit:

    return mOrigStreamListener->(...);

::: toolkit/modules/addons/nsWebRequestListener.cpp:45
(Diff revision 3)
> +  NS_ASSERTION(mOrigStreamListener, "Should have mOrigStreamListener");
> +  nsresult rv = mOrigStreamListener->OnStopRequest(request, aCtxt, aStatus);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  NS_ASSERTION(mTargetStreamListener, "Should have mTargetStreamListener");
> +  rv = mTargetStreamListener->OnStopRequest(request, aCtxt, aStatus);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  return rv;

Same comments as above.

::: toolkit/modules/addons/nsWebRequestListener.cpp:61
(Diff revision 3)
> +  NS_ASSERTION(mOrigStreamListener, "Should have mOrigStreamListener");
> +  nsresult rv = mOrigStreamListener->OnDataAvailable(request, aCtxt, inStr, sourceOffset, count);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  return rv;

Same comments here, too.
Attachment #8850158 - Flags: review?(kmaglione+bmo) → review+
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2e58d1f1ac27
implement off-main-thread delivery with start/stop/error listeners, r=kmag
https://hg.mozilla.org/mozilla-central/rev/2e58d1f1ac27
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Shane, Kris, is 54 affected? Is this upliftable?
Flags: needinfo?(mixedpuppy)
Flags: needinfo?(kmaglione+bmo)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #18)
> Shane, Kris, is 54 affected? Is this upliftable?

It is affected, I'm uncertain if it's upliftable without trying to apply it, but it would be close.  

I'm not certain there is value in uplifting this.  It would only be of benefit if webextensions using webrequest are loaded, and we've lived with main-thread request handling thus far.
Flags: needinfo?(mixedpuppy)
Flags: needinfo?(kmaglione+bmo)
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.