Support a webRequest.onResponseData event to filter HTTP response bytes as they come

NEW
Assigned to
(Needinfo from 2 people)

Status

()

Toolkit
WebExtensions: Request Handling
P2
normal
2 years ago
15 days ago

People

(Reporter: mao, Assigned: kmag, NeedInfo)

Tracking

(Blocks: 2 bugs, {dev-doc-needed})

unspecified
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [webRequest] triaged, [we-enterprise])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(8 attachments, 3 obsolete attachments)

59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details | Review
59 bytes, text/x-review-board-request
baku
: review+
Details | Review
59 bytes, text/x-review-board-request
mixedpuppy
: review+
baku
: review+
Details | Review
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details | Review
59 bytes, text/x-review-board-request
mixedpuppy
: review+
baku
: review+
Details | Review
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details | Review
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details | Review
59 bytes, text/x-review-board-request
mystor
: review-
Details | Review
(Reporter)

Description

2 years ago
Extensions like NoScript or  EPUBReader ( https://addons.mozilla.org/en-US/firefox/addon/epubreader ), whose developer has recently contacted me to investigate the feasibility of a WebExtensions migration, need to manipulate HTTP responses on the fly and possibly divert them from their original consumer.

The Chrome extensions API does support a requestBody property in the webRequest.onBeforeRequest event (which, strangely enough, represents raw data a single array of bytes, with no way to consume it in chunks when it's a very large upload), allows for request and response headers manipulation, but has no way to analyze, let alone modify, the response payload.

Like requestBody, responseChunk be potentially very large and its consumers may require asynchronous processing not to block the parent process, therefore this too depends on bug 1254204.



Therefore I propose to extend the webRequest API with an additional event, tentatively called onResponseData, which is called one or more times as the response data is streamed down. The details object would contain the usual properties, and especially requestId, allowing clients to keep per-request state as needed, plus a new property called responseChunk, i.e. an array filled with the response bytes in chunks, as they're made available to the onDataAvailable() method of nsIStreamListener attached to the underlying HTTP channel.

Since the webRequest API guarantees that either the onCompleted or the onErrorOccurred listener is called when a request is done, we don't need any special EOF signaling: the client would handle its cleanup operations in those events' callbacks.
(Reporter)

Updated

a year ago
No longer blocks: 1201979

Updated

a year ago
Whiteboard: [webRequest] → [webRequest] triaged

Updated

a year ago
Flags: blocking-webextensions+
Priority: -- → P2

Updated

a year ago
Flags: blocking-webextensions+

Comment 1

a year ago
Please update us on the progress of onResponseData implementation

Our add-on needs to read incoming data on the fly

Updated

10 months ago
Component: WebExtensions: Untriaged → WebExtensions: Request Handling
Blocks: 1310316
We need to make sure this work doesn't cause performance problems.  We've done a lot of work in necko to allow off-main thread delivery (see bug 497003)--currently the HTML parser and the image parser both use that to improve performance.  Currently that work gets disabled if we detect that the nsITraceableChannel (the standard--and IIUC the only--way to intercept and modify data before OnDataAvailable is called on the original listener). See bug 908398 and bug 908405)

So we want to 1) discourage use of this API unless it's really needed (and I get that it may be for some use cases), and 2) make sure that we don't bake in any performance hit when these addons aren't being used.  I'm guessing we'll get #2 for free--that's how things already work (necko does a search through the listener chain to make sure all listeners are off-main-ready before switching to off-main delivery: the presence of any JS-implemented listeners (added by nsITraceableChannel), for instance, causes code to stay main-thread only).

So maybe there's no issue here--but I want to make sure. SC, can you keep an eye on the architecture used here and make sure we don't break off-main delivery any more than necessary?  If you need help figuring that out ask Honza.
Flags: needinfo?(schien)
(Assignee)

Comment 3

9 months ago
The extension code that's consuming this will be running in its own process, so we can probably implement it in a way that's compatible with off-main-thread delivery. Either way, I definitely agree that we want to discourage this whenever it's not absolutely necessary.

Comment 4

9 months ago
OOP addons are still single-threaded, maybe it should be handled by workers?
(Assignee)

Comment 5

9 months ago
(In reply to The 8472 from comment #4)
> OOP addons are still single-threaded, maybe it should be handled by workers?

It's not so much an issue of them being single-threaded as it is of not blocking the main UI thread while the data is being processed. Granted, they'd still block any UI that's running in the extension process, but that's a much smaller issue.

It definitely would be nice to be able to push these kinds of APIs into something like a service worker in the future, but that's a ways down the road.

Comment 6

9 months ago
Not quite as simple. If an extension takes time to mangle each request this can also slow down page loads. Parallelizing it could improve user experience.
(Assignee)

Comment 7

9 months ago
If the data processing takes longer than the data transfer from the network, that's probably true. But workers don't buy us anything unless we somehow spawned multiple workers to handle parallel requests (which presents its own whole set of problems), and we'd like to discourage extensions from doing things like that in any case.

Comment 8

9 months ago
Yes, the idea was to have multiple workers. But even with a single worker it would still improve throughput if multiple addons use this feature.
(Assignee)

Comment 9

9 months ago
Only if they happen to be running in the same process. But, again, if we have more consumers of this API than we have extension processes, or those extensions process data significantly slower than we can read it from the network, we're doing something wrong. That's not how this API is meant to be used.
(Assignee)

Updated

8 months ago
Duplicate of this bug: 1325278

Comment 11

8 months ago
This bug is blocking me from porting my extension to WebExtensions. I have an extension that scans JPEGs that are coming over the wire to alert on them containing EXIF GPS information (which is potentially privacy-compromising). My extension currently uses an nsIObserver and snoops incoming responses with JPEG MIME types. As far as I can tell, there isn't a way to do this using WebExtensions.

The source is here: https://github.com/allanlw/gps-detect/ and the listing is: https://addons.mozilla.org/en-US/firefox/addon/gpsdetect/

Updated

7 months ago
webextensions: --- → ?

Updated

7 months ago
Assignee: g.maone → nobody

Updated

6 months ago
Whiteboard: [webRequest] triaged → [webRequest] triaged, [we-enterprise]
(Assignee)

Updated

6 months ago
Duplicate of this bug: 1340802

Comment 13

6 months ago
Kris - Thanks for merging the bugs. My addon Integrated Inbox will cease to work, as it sounds like is the case for many others, unless this is addressed. 

Have a great weekend,
Michael
(Assignee)

Updated

6 months ago
Duplicate of this bug: 1344561

Comment 15

5 months ago
Created attachment 8845654 [details] [diff] [review]
webrequest_oda_uint8.diff

Would something like this be what we're looking for, once bug 1261585 is fixed?

This patch just adds an onResponseData listener to WebRequest, which can optionally be "blocking" if response-rewriting is desired rather than simple inspection. It passes the data into the listener as a Uint8Array named responseBytes.

It just builds on the existing StartStopListener::OnDataAvailable, only doing extra work if an onResponseData listener was detected (hopefully I did that correctly).
Attachment #8845654 - Flags: review?(kmaglione+bmo)

Comment 16

5 months ago
Created attachment 8846323 [details] [diff] [review]
webrequest_oda_uint8.diff

Here's a fresh version of the last patch, which changes the onDataAvailable handler to be a bit more solid, now that I've had a chance to test it a bit more thoroughly.
Attachment #8845654 - Attachment is obsolete: true
Attachment #8845654 - Flags: review?(kmaglione+bmo)
Attachment #8846323 - Flags: feedback?(kmaglione+bmo)
I started to look at bug 1326298 and was going to put some focus on that.  I'd rather not see this land simply to have to convert it to native code right away.
(Assignee)

Comment 18

5 months ago
Comment on attachment 8846323 [details] [diff] [review]
webrequest_oda_uint8.diff

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

Sorry I didn't see this sooner. As commented elsewhere in the bug, this needs to be implemented in C++, with careful attention to thread safety and performance. I thought the bug was already assigned to me.
Attachment #8846323 - Flags: feedback?(kmaglione+bmo) → feedback-
(Assignee)

Updated

5 months ago
Assignee: nobody → kmaglione+bmo
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 27

5 months ago
Comment on attachment 8846323 [details] [diff] [review]
webrequest_oda_uint8.diff

Oh, no problem! One way or the other, I figured it would be worth providing a patch for others to experiment with if they need such an API for their extensions.
Attachment #8846323 - Attachment is obsolete: true

Comment 28

5 months ago
mozreview-review
Comment on attachment 8850697 [details]
Bug 1255894: Part 1 - Allow applying content conversions in parent rather than child when necessary.

https://reviewboard.mozilla.org/r/123230/#review125854

This is not going to work. We do not apply conversion always. Only when the last listener in a listener chain (that is URILoader ans ExternalHelper) is called we know whether to apply conversion or not, e.g. this has to do with saving gzip files as gzip. And then there is UnknownDecoder that calls the last listener only when some OnDataAvailable is called, e.g. respons is already transfer to the child so nsITracableChannel cannot modify it. For more info please read bug 1261585.

So let me finish bug 1342386 and 1261585.
Bug 1261585 is done, it is only waiting for bug 1342386.
I have a patch for bug 1342386, but i did not have time to test it yet. It touch some very old code that I am afraid to break.(This bug transforms UnknownDecoder from a stream listener to a sniffer).
Attachment #8850697 - Flags: review?(dd.mozilla) → review-
Sorry, I'm not sure when I'll be able to get to this Kris. The last few days I was really busy preparing for a workweek next week, and I expect I won't have much time at the workweek itself. If there's anyone else who can review, please feel free to give it to them. The earliest I can likely review is April 3rd.
:baku might be a good fit.
(Assignee)

Comment 31

5 months ago
Shane and I will also be at a work week next week. I'll probably have time to be responsive, but if Shane doesn't think he'll have time to get to it either, it can probably wait until after.
Flags: needinfo?(mixedpuppy)
I've been digging through a couple patches but am wanting to wrap my head around the entirety before reviewing any single patch.  Was last looking at all the state engine stuff.  I don't expect to have time over work week to finish reviews.
Flags: needinfo?(mixedpuppy)
(Assignee)

Updated

5 months ago
Attachment #8850699 - Flags: review?(wmccloskey) → review?(amarchesini)
Attachment #8850700 - Flags: review?(wmccloskey) → review?(amarchesini)
Attachment #8850702 - Flags: review?(wmccloskey) → review?(amarchesini)
(Assignee)

Comment 33

5 months ago
Baku, Bill suggested you might be a good person to review this. Let me know if you have any questions, or you don't feel like you're the right person.

Thanks!

Updated

5 months ago
webextensions: ? → +
Comment on attachment 8850699 [details]
Bug 1255894: Part 3 - Create skeleton IPDL framework for OOP stream filters.

https://reviewboard.mozilla.org/r/123234/#review127646

This needs a better name. PStream looks like IPCStream that is about nsIInputStream.
Can you rename it?
I guess the fun part is in some other patch :)

::: toolkit/components/extensions/webrequest/PStreamFilter.ipdl:9
(Diff revision 1)
> +
> +include protocol PBackground;
> +
> +namespace mozilla {
> +namespace extensions {
> +

Add a comment about what this all is about.
Attachment #8850699 - Flags: review?(amarchesini) → review+
Comment on attachment 8850700 [details]
Bug 1255894: Part 4 - Implement StreamFilter IPC protocol.

https://reviewboard.mozilla.org/r/123236/#review127664

Ok, This is a complex patch. I need to review it more than once.
Here the first comments. I have a couple of questions:

1. Why do we need to have a double logic in the child and in the parent? Can we have it only in the child?
I would like to have a 'simpler' parent that does only what the child requires. Basically, I think we can have the parent without mState.
If we do that, we just need a boolean to know if the channel is suspended or not in case we need to resume it.

2. We must be sure that we don't send messages if the child crashes. ActorDestroyed() should be used to avoid that. Currently this is not implemented.

3. IO Thread or main-thread? There are a couple of naming issues.

::: toolkit/components/extensions/webrequest/PStreamFilter.ipdl:36
(Diff revision 1)
> +
> +  async StartRequest();
> +  async Data(uint8_t[] data);
> +  async StopRequest(nsresult aStatus);
> +
>    async __delete__();

Can you write here a comment explaining why sending __delete__ in the child is safe? What does prevent the parent to send another message in the meantime?

::: toolkit/components/extensions/webrequest/StreamFilterBase.h:22
(Diff revision 1)
> +{
> +public:
> +  typedef nsTArray<uint8_t> Data;
> +
> +protected:
> +  class BufferedData : public LinkedListElement<BufferedData> {

{ in a new line

::: toolkit/components/extensions/webrequest/StreamFilterBase.h:32
(Diff revision 1)
> +  };
> +
> +  LinkedList<BufferedData> mBufferedData;
> +
> +  inline void
> +  BufferData(Data&& aData) {

same here. \n{

::: toolkit/components/extensions/webrequest/StreamFilterChild.h:30
(Diff revision 1)
>  {
>  public:
>    NS_INLINE_DECL_REFCOUNTING(StreamFilterChild)
>  
> -  StreamFilterChild() {}
> +  StreamFilterChild()
> +    : mState(State::Uninitialized)

mNextState ?

::: toolkit/components/extensions/webrequest/StreamFilterChild.h:34
(Diff revision 1)
> -  StreamFilterChild() {}
> +  StreamFilterChild()
> +    : mState(State::Uninitialized)
> +    , mReceivedOnStop(false)
> +  {}
> +
> +  enum class State {

can you write comments explaining each state?

::: toolkit/components/extensions/webrequest/StreamFilterChild.h:49
(Diff revision 1)
> +    Disconnecting,
> +    Disconnected,
> +    Error,
> +  };
> +
> +  void Suspend(ErrorResult& rv);

aRv

::: toolkit/components/extensions/webrequest/StreamFilterChild.h:52
(Diff revision 1)
> +  };
> +
> +  void Suspend(ErrorResult& rv);
> +  void Resume(ErrorResult& rv);
> +  void Disconnect(ErrorResult& rv);
> +  void Close(ErrorResult& rv);

aRv to all these methods.

::: toolkit/components/extensions/webrequest/StreamFilterChild.h:57
(Diff revision 1)
> +  void Close(ErrorResult& rv);
> +  void Cleanup();
> +
> +  void Write(Data&& aData, ErrorResult& aRv);
> +
> +  inline State GetState() const

inline is not needed in class definition

::: toolkit/components/extensions/webrequest/StreamFilterChild.h:66
(Diff revision 1)
>  
>  protected:
> +  virtual IPCResult RecvInitialized(const bool& aSuccess) override;
> +
> +  virtual IPCResult RecvStartRequest() override;
> +  virtual IPCResult RecvData(Data&& data) override;

aData

::: toolkit/components/extensions/webrequest/StreamFilterChild.cpp:34
(Diff revision 1)
> +  case State::Disconnected:
> +    break;
> +
> +  default:
> +    ErrorResult rv;
> +    Disconnect(rv);

If you don't care about the result, use IgnoreErrorResult instead. the DTOR of ErrorResult, if it contains and error, does MOZ_CRASH.

::: toolkit/components/extensions/webrequest/StreamFilterChild.cpp:44
(Diff revision 1)
> +/*****************************************************************************
> + * State change methods
> + *****************************************************************************/
> +
> +void
> +StreamFilterChild::Suspend(ErrorResult& rv)

aRv, here and elsewhere.

::: toolkit/components/extensions/webrequest/StreamFilterChild.cpp:55
(Diff revision 1)
> +
> +    SendSuspend();
> +    break;
> +
> +  case State::Suspending:
> +    switch (mNextState) {

instead having 2 nested switch, what about:
if (mNextStep == State::Suspended && mNextState == State::Resuming) { ... } else { aRv.Throw(NS_ERROR_FAILURE); }

::: toolkit/components/extensions/webrequest/StreamFilterChild.cpp:62
(Diff revision 1)
> +    case State::Resuming:
> +      mNextState = State::Suspended;
> +      break;
> +
> +    default:
> +      rv = NS_ERROR_FAILURE;

rv.Throw(NS_ERROR_FAILURE);

::: toolkit/components/extensions/webrequest/StreamFilterChild.cpp:68
(Diff revision 1)
> +      return;
> +    }
> +    break;
> +
> +  case State::Resuming:
> +    switch (mNextState) {

same comments here.

::: toolkit/components/extensions/webrequest/StreamFilterChild.cpp:84
(Diff revision 1)
> +
> +  case State::Suspended:
> +    break;
> +
> +  default:
> +    rv = NS_ERROR_FAILURE;

aRv.Throw...

::: toolkit/components/extensions/webrequest/StreamFilterChild.cpp:90
(Diff revision 1)
> +    break;
> +  }
> +}
> +
> +void
> +StreamFilterChild::Resume(ErrorResult& rv)

aRv

::: toolkit/components/extensions/webrequest/StreamFilterChild.cpp:91
(Diff revision 1)
> +  }
> +}
> +
> +void
> +StreamFilterChild::Resume(ErrorResult& rv)
> +{

1. aRv
2. can we add MOZ_ASSERT about the mState?

::: toolkit/components/extensions/webrequest/StreamFilterChild.cpp:126
(Diff revision 1)
> +
> +  FlushBufferedData();
> +}
> +
> +void
> +StreamFilterChild::Disconnect(ErrorResult& rv)

aRv

::: toolkit/components/extensions/webrequest/StreamFilterChild.cpp:326
(Diff revision 1)
> +
> +    default:
> +      aRv = NS_ERROR_FAILURE;
> +      return;
> +    }
> +    break;

Why are we calling SendWrite() when we are in suspending or suspended mode? (I still need to read the parent side).

It seems that basically we are sending data always if we are not disconnected/closed.

::: toolkit/components/extensions/webrequest/StreamFilterParent.h:39
(Diff revision 1)
>    NS_DECL_THREADSAFE_ISUPPORTS
>  
>    explicit StreamFilterParent(uint64_t aChannelId, const nsAString& aAddonId);
>  
> +  enum class State {
> +    Uninitialized,

Comments here.

::: toolkit/components/extensions/webrequest/StreamFilterParent.h:60
(Diff revision 1)
>    void Init(already_AddRefed<nsIContentParent> aContentParent);
>  
>  protected:
>    virtual ~StreamFilterParent();
>  
> +  virtual IPCResult RecvWrite(Data&& data) override;

aData

::: toolkit/components/extensions/webrequest/StreamFilterParent.h:83
(Diff revision 1)
>    virtual void ActorDestroy(ActorDestroyReason aWhy) override;
>  
> +  void Broken();
> +
> +  inline void
> +  CheckResult(bool result)

aResult

::: toolkit/components/extensions/webrequest/StreamFilterParent.h:110
(Diff revision 1)
> +    MOZ_ASSERT(NS_IsMainThread());
> +  }
> +
> +  // Ensures that a strong reference to this instance is held alive while a
> +  // method-based or closure-based runnable is being dispatched.
> +  class KungFuDeathGrip : public Runnable {

{ in a new line

::: toolkit/components/extensions/webrequest/StreamFilterParent.h:112
(Diff revision 1)
> +
> +  // Ensures that a strong reference to this instance is held alive while a
> +  // method-based or closure-based runnable is being dispatched.
> +  class KungFuDeathGrip : public Runnable {
> +  public:
> +    KungFuDeathGrip(StreamFilterParent* aGrip, already_AddRefed<nsIRunnable> aRunnable);

make it private

::: toolkit/components/extensions/webrequest/StreamFilterParent.h:126
(Diff revision 1)
> +    }
> +
> +    NS_IMETHOD Run() override;
> +
> +  protected:
> +    virtual ~KungFuDeathGrip();

= default

::: toolkit/components/extensions/webrequest/StreamFilterParent.h:130
(Diff revision 1)
> +  protected:
> +    virtual ~KungFuDeathGrip();
> +
> +  private:
> +    RefPtr<StreamFilterParent> mGrip;
> +    RefPtr<nsIRunnable> mRunnable;

Runnable. Why nsIRunnable? If it is nsIRunnable, should it be nsCOMPtr?

::: toolkit/components/extensions/webrequest/StreamFilterParent.h:135
(Diff revision 1)
> +    RefPtr<nsIRunnable> mRunnable;
> +  };
> +
> +  // Dispatches a runnable to the given event target, while making sure a strong
> +  // reference to this instance is kept alive until it completes.
> +  inline void

remove inline here an elsewhere.

::: toolkit/components/extensions/webrequest/StreamFilterParent.h:138
(Diff revision 1)
> +  // Dispatches a runnable to the given event target, while making sure a strong
> +  // reference to this instance is kept alive until it completes.
> +  inline void
> +  SafeDispatch(nsIEventTarget* aEventTarget, already_AddRefed<nsIRunnable> aRunnable)
> +  {
> +      nsCOMPtr<nsIRunnable> runnable = aRunnable;

indentation

::: toolkit/components/extensions/webrequest/StreamFilterParent.h:158
(Diff revision 1)
> +
> +  template<typename Function>
> +  inline void
> +  RunOnWorkerThread(Function&& aFunc)
> +  {
> +    SafeDispatch(mWorkerThread,

same line?

::: toolkit/components/extensions/webrequest/StreamFilterParent.h:166
(Diff revision 1)
> +
> +  template<typename Function>
> +  inline void
> +  RunOnIOThread(Function&& aFunc)
> +  {
> +    SafeDispatch(mIOThread,

same line?

::: toolkit/components/extensions/webrequest/StreamFilterParent.cpp:46
(Diff revision 1)
> + *****************************************************************************/
>  
>  StreamFilterParent::StreamFilterParent(uint64_t aChannelId, const nsAString& aAddonId)
>    : mChannelId(aChannelId)
>    , mAddonId(NS_Atomize(aAddonId))
> +  , mWorkerThread(NS_GetCurrentThread())

Call it PBackgroundThread

::: toolkit/components/extensions/webrequest/StreamFilterParent.cpp:47
(Diff revision 1)
>  
>  StreamFilterParent::StreamFilterParent(uint64_t aChannelId, const nsAString& aAddonId)
>    : mChannelId(aChannelId)
>    , mAddonId(NS_Atomize(aAddonId))
> +  , mWorkerThread(NS_GetCurrentThread())
> +  , mIOThread(do_GetMainThread())

main thread? Why?

::: toolkit/components/extensions/webrequest/StreamFilterParent.cpp:64
(Diff revision 1)
>  void
>  StreamFilterParent::Init(already_AddRefed<nsIContentParent> aContentParent)
>  {
> -  nsCOMPtr<nsIContentParent> contentParent = aContentParent;
> -  NS_ReleaseOnMainThread(contentParent.forget());
> +  AssertIsWorkerThread();
> +
> +  nsIContentParent* contentParent = aContentParent.take();

Instead having this raw pointer, can you do something similar to what we have in BackgroundImplParent with CheckPrincipalRunnable?

::: toolkit/components/extensions/webrequest/StreamFilterParent.cpp:71
(Diff revision 1)
> +    DoInit(contentParent);
> +  });
> +}
> +
> +void
> +StreamFilterParent::DoInit(nsIContentParent* aContentParent)

call it InitOnMainThread

::: toolkit/components/extensions/webrequest/StreamFilterParent.cpp:79
(Diff revision 1)
> +
> +  RefPtr<nsIContentParent> contentParent = dont_AddRef(aContentParent);
> +
> +  bool success = false;
> +  auto guard = MakeScopeExit([&] {
> +    RunOnWorkerThread([=] {

RunOnBackgroundThread

::: toolkit/components/extensions/webrequest/StreamFilterParent.cpp:103
(Diff revision 1)
> + *****************************************************************************/
> +
> +void
> +StreamFilterParent::Broken()
> +{
> +  AssertIsWorkerThread();

rename worker to backgroundThread here. 'Worker' reminds DOM Workers.

::: toolkit/components/extensions/webrequest/StreamFilterParent.cpp:139
(Diff revision 1)
> +
> +  if (mState == State::TransferringData) {
> +    RunOnMainThread([this] {
> +      mChannel->Suspend();
> +    });
> +    RunOnWorkerThread([this] {

why a runnable here?

::: toolkit/components/extensions/webrequest/StreamFilterParent.cpp:141
(Diff revision 1)
> +    RunOnMainThread([this] {
> +      mChannel->Suspend();
> +    });
> +    RunOnWorkerThread([this] {
> +      mState = State::Suspended;
> +      CheckResult(SendSuspended());

This is wrong. here you have a runnable. But what about if the child is already crashed? You should deal with ActorDestroyed().

::: toolkit/components/extensions/webrequest/StreamFilterParent.cpp:156
(Diff revision 1)
> +
> +  if (mState == State::Suspended) {
> +    RunOnMainThread([this] {
> +      mChannel->Resume();
> +    });
> +    RunOnWorkerThread([this] {

same here.
Attachment #8850700 - Flags: review?(amarchesini) → review-
Comment on attachment 8850702 [details]
Bug 1255894: Part 6 - Implement StreamFilter DOM bindings.

https://reviewboard.mozilla.org/r/123240/#review128652

::: dom/webidl/StreamFilter.webidl:6
(Diff revision 1)
> +/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/.
> + */
> +

Documentation, URL of what this spec is about. Or a good comment explaining why we need it and where it is used.

::: dom/webidl/StreamFilterDataEvent.webidl:5
(Diff revision 1)
> +/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

documentation here as well.

::: toolkit/components/extensions/webrequest/StreamFilter.h:14
(Diff revision 1)
> +
> +#include "mozilla/dom/BindingDeclarations.h"
> +#include "mozilla/dom/StreamFilterBinding.h"
> +#include "mozilla/extensions/StreamFilterChild.h"
> +
> +#include "jsapi.h"

why do you need this?

::: toolkit/components/extensions/webrequest/StreamFilter.h:43
(Diff revision 1)
> +  static already_AddRefed<StreamFilter>
> +  Create(GlobalObject& global,
> +         uint64_t aRequestId,
> +         const nsAString& aAddonId);
> +
> +  explicit StreamFilter(nsIGlobalObject* aParent,

no explicit here. we have more than 1 argument.

::: toolkit/components/extensions/webrequest/StreamFilter.h:60
(Diff revision 1)
> +  void GetError(nsAString& aError)
> +  {
> +    aError = mError;
> +  }
> +
> +  inline StreamFilterStatus

no inline if it's already inline

::: toolkit/components/extensions/webrequest/StreamFilter.h:61
(Diff revision 1)
> +  {
> +    aError = mError;
> +  }
> +
> +  inline StreamFilterStatus
> +  Status() const {

{ in a new line

::: toolkit/components/extensions/webrequest/StreamFilter.h:66
(Diff revision 1)
> +  Status() const {
> +    MOZ_ASSERT(mActor);
> +    return mActor->Status();
> +  }
> +
> +  inline void Suspend(ErrorResult& aRv)

same comment about the inline

::: toolkit/components/extensions/webrequest/StreamFilter.h:74
(Diff revision 1)
> +    return mActor->Suspend(aRv);
> +  }
> +
> +  inline void Resume(ErrorResult& aRv)
> +  {
> +    MOZ_ASSERT(mActor);

nothing guarantees that mActor exists.

::: toolkit/components/extensions/webrequest/StreamFilter.cpp:115
(Diff revision 1)
> + *****************************************************************************/
> +
> +void
> +StreamFilter::Write(const ArrayBufferOrUint8Array& aData, ErrorResult& aRv)
> +{
> +  nsTArray<uint8_t> data;

make it FallibleTArray

::: toolkit/components/extensions/webrequest/StreamFilter.cpp:121
(Diff revision 1)
> +
> +  if (aData.IsArrayBuffer()) {
> +    const ArrayBuffer& buffer = aData.GetAsArrayBuffer();
> +
> +    buffer.ComputeLengthAndData();
> +    data.SetLength(buffer.Length());

SetLength() could fail if FallibleTArray. return OOM.

::: toolkit/components/extensions/webrequest/StreamFilter.cpp:127
(Diff revision 1)
> +    memcpy(data.Elements(), buffer.Data(), buffer.Length());
> +  } else if (aData.IsUint8Array()) {
> +    const Uint8Array& array = aData.GetAsUint8Array();
> +
> +    array.ComputeLengthAndData();
> +    data.SetLength(array.Length());

same here.

::: toolkit/components/extensions/webrequest/StreamFilter.cpp:145
(Diff revision 1)
> +
> +void
> +StreamFilter::FireEvent(const nsAString& aType)
> +{
> +  AutoEntryScript aes(mParent, "StreamFilter event");
> +  JSContext* cx = aes.cx();

remove this.

::: toolkit/components/extensions/webrequest/StreamFilter.cpp:147
(Diff revision 1)
> +StreamFilter::FireEvent(const nsAString& aType)
> +{
> +  AutoEntryScript aes(mParent, "StreamFilter event");
> +  JSContext* cx = aes.cx();
> +
> +  RootedDictionary<EventInit> init(cx);

it doesn't need to be rooted.

::: toolkit/components/extensions/webrequest/StreamFilter.cpp:167
(Diff revision 1)
> +  JSContext* cx = aes.cx();
> +
> +  RootedDictionary<StreamFilterDataEventInit> init(cx);
> +  init.mBubbles = false;
> +  init.mCancelable = false;
> +  init.mData.Init(ArrayBuffer::Create(cx, this, aData.Length(), aData.Elements()));

ArrayBuffer::Create can fail.

::: toolkit/components/extensions/webrequest/StreamFilterChild.cpp:368
(Diff revision 1)
> +  case State::Resuming:
> +  case State::Suspending:
> +    switch (mNextState) {
> +    case State::TransferringData:
> +    case State::Resuming:
> +      return StreamFilterStatus::Transferringdata;

This means that if I do:

foo.resume();
console.log(foo.status) -> it returns transferringData

::: toolkit/components/extensions/webrequest/StreamFilterEvents.h:42
(Diff revision 1)
> +  static already_AddRefed<StreamFilterDataEvent>
> +  Constructor(EventTarget* aEventTarget,
> +              const nsAString& aType,
> +              const StreamFilterDataEventInit& aParam);
> +
> +  static inline already_AddRefed<StreamFilterDataEvent>

inline

::: toolkit/components/extensions/webrequest/StreamFilterEvents.h:52
(Diff revision 1)
> +  {
> +    nsCOMPtr<EventTarget> target = do_QueryInterface(aGlobal.GetAsSupports());
> +    return Constructor(target, aType, aParam);
> +  }
> +
> +  inline void GetData(JSContext* aCx, JS::MutableHandleObject aResult) {

1. inline
2. {
Attachment #8850702 - Flags: review?(amarchesini) → review-
(Assignee)

Comment 37

5 months ago
mozreview-review-reply
Comment on attachment 8850700 [details]
Bug 1255894: Part 4 - Implement StreamFilter IPC protocol.

https://reviewboard.mozilla.org/r/123236/#review127664

1) I tried to keep as much of the state logic in the child as possible. There are still a lot of things we need to keep track of in the parent, though, particularly for the sake of disconnecting the filter from the child and having communication continue unfiltered in the parent.

2) You're right, I meant to implement ActorDestroyed and handle it the way we handle a failure to send.

3) IO and main thread are two separate threads. The IO thread is the thread where channel IO happens. That's usually the main thread, but it can be retargeted for some channels. The main thread is where we need to do all non-IO interaction with the channel.

> Can you write here a comment explaining why sending __delete__ in the child is safe? What does prevent the parent to send another message in the meantime?

We don't send delete in the child. The child sends `Close()` to the parent, and then the parent sends `__delete__` to the child when it's finished.

> mNextState ?

It shouldn't matter. Any use of `mNextState` before it's initialized elsewhere would be an error, but I suppose initializing it to a sentinel value wouldn't be a bad idea.

> inline is not needed in class definition

I know, but I generally prefer to include it anyway. But I don't really have a strong opinion.

> If you don't care about the result, use IgnoreErrorResult instead. the DTOR of ErrorResult, if it contains and error, does MOZ_CRASH.

I think crashing on error is probably what we want here. `Disconnect()` should only ever return an error if we call it in an invalid state.

> instead having 2 nested switch, what about:
> if (mNextStep == State::Suspended && mNextState == State::Resuming) { ... } else { aRv.Throw(NS_ERROR_FAILURE); }

I don't really have a strong opinion on it. The nested switch is consistent with the other places where we need to do more complicated checks, and has the benefit of compiler warnings if we forget a state. But I'd be willing to switch to an `if`.

> 1. aRv
> 2. can we add MOZ_ASSERT about the mState?

What assert would we add? This is called from JS, so we really want to throw rather than crash if the state isn't what we expect.

> Why are we calling SendWrite() when we are in suspending or suspended mode? (I still need to read the parent side).
> 
> It seems that basically we are sending data always if we are not disconnected/closed.

So, basically, `Suspend()` only stops new incoming data from being dispatched to the JS listener. When the channel is suspended, the JS side is still allowed to write its own data to the channel.

Once it's called disconnect(), though, the filter is effectively dead. All data passes through on the parent side unfiltered, and the child isn't allowed to write additional data.

> Runnable. Why nsIRunnable? If it is nsIRunnable, should it be nsCOMPtr?

I guess it probably can be `Runnable`. I'm not sure if we currently use it with any other kind of `nsIRunnable`. It should have been `nsCOMPtr`, though, yeah.

> main thread? Why?

Because that's the default IO thread for all channels. It's never redirected to other threads until someone calls `RetargetDeliveryTo()` on the channel.

> Instead having this raw pointer, can you do something similar to what we have in BackgroundImplParent with CheckPrincipalRunnable?

I could, but it gets verbose very fast when we start creating special-purpose runnable classes, and closures don't work well with smart pointers.

> rename worker to backgroundThread here. 'Worker' reminds DOM Workers.

I mostly chose the terminology for consistency with the storage actors, but I'm not especially fond of it either.

> why a runnable here?

Oh, that was originally inside the `RunOnMainThread` runnable, so we didn't wind up doing it until after the channel was actually suspended. I'm no sure why I moved it out.

> This is wrong. here you have a runnable. But what about if the child is already crashed? You should deal with ActorDestroyed().

Hm. That's a good point.
(Assignee)

Comment 38

5 months ago
mozreview-review-reply
Comment on attachment 8850702 [details]
Bug 1255894: Part 6 - Implement StreamFilter DOM bindings.

https://reviewboard.mozilla.org/r/123240/#review128652

> why do you need this?

I probably don't anymore. A previous version needed it for dealing with typed arrays.

> no explicit here. we have more than 1 argument.

Implicit constructures with multiple arguments are allowed in C++11

> nothing guarantees that mActor exists.

Hm. You're right. Originally I wasn't exposing the object to JS until it was initialized, but now that isn't guaranteed.

> This means that if I do:
> 
> foo.resume();
> console.log(foo.status) -> it returns transferringData

Right, that's intended. From the point of view of the JS consumer, there are no intermediate states. We just need to track them internally so that we don't try to change to a new state before the last state change call is complete.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

5 months ago
Attachment #8850697 - Attachment is obsolete: true

Comment 45

5 months ago
mozreview-review
Comment on attachment 8850698 [details]
Bug 1255894: Part 2 - Add mozIWebRequestService service for tracking filtered requests.

https://reviewboard.mozilla.org/r/123232/#review129576
Attachment #8850698 - Flags: review?(mixedpuppy) → review+

Comment 46

5 months ago
mozreview-review
Comment on attachment 8850704 [details]
Bug 1255894: Part 8 - Add tests for response stream filtering.

https://reviewboard.mozilla.org/r/123244/#review129614
Attachment #8850704 - Flags: review?(mixedpuppy) → review+

Comment 47

5 months ago
mozreview-review
Comment on attachment 8850703 [details]
Bug 1255894: Part 7 - Expose response stream filtering via the webRequest API.

https://reviewboard.mozilla.org/r/123242/#review129578

::: toolkit/components/extensions/schemas/web_request.json:210
(Diff revision 2)
> +      },
> +      {
> +        "name": "filterResponseData",
> +        "permissions": ["webRequestBlocking"],
> +        "type": "function",
> +        "description": "...",

add description
Attachment #8850703 - Flags: review?(mixedpuppy) → review+

Comment 48

5 months ago
mozreview-review
Comment on attachment 8850700 [details]
Bug 1255894: Part 4 - Implement StreamFilter IPC protocol.

https://reviewboard.mozilla.org/r/123236/#review130056

That took a bit to wrap ones head around, but I feel pretty comfortable with all the state logic at this point.
Attachment #8850700 - Flags: review?(mixedpuppy) → review+

Comment 49

5 months ago
mozreview-review
Comment on attachment 8850702 [details]
Bug 1255894: Part 6 - Implement StreamFilter DOM bindings.

https://reviewboard.mozilla.org/r/123240/#review130058

There's a lot in this that I haven't touched before so I don't feel comfortable reviewing the code, baku can finish that up.  From what I understand is going on here, I feel ok with what it does in the context of the overall patch set.
Attachment #8850702 - Flags: review?(mixedpuppy) → review+

Comment 50

5 months ago
mozreview-review
Comment on attachment 8850701 [details]
Bug 1255894: Part 5 - Hook up StreamFilterParent as a stream listener.

https://reviewboard.mozilla.org/r/123238/#review130064

r+ for my part
Attachment #8850701 - Flags: review?(mixedpuppy) → review+
(Assignee)

Comment 51

5 months ago
Cheers. I owe all of you drinks :)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 60

4 months ago
mozreview-review
Comment on attachment 8856132 [details]
Bug 1255894: Allow capturing `this` in lambda expressions when marked safe.

https://reviewboard.mozilla.org/r/128076/#review130936

I don't like this solution to the problem of avoiding raw pointer inside lambda static analysis. 

a) I don't think that the decision of whether or not a raw pointer to a type may be captured should be based on a type level flag. Instead it should be based on the call site.
b) CustomTypeAnnotation is the wrong way to implement this. This implements an inheritable attribute, which is inherited from members and base classes, which is definitely not what we want here (as a member being safe for static analysis doesn't imply that the outer type is at all).

I would suggest for now using a workaround in your code similar to this one (http://searchfox.org/mozilla-central/rev/eace920a0372051a11d8d275bd9b8f14f3024ecd/dom/media/MediaEventSource.h#143-154) in the media code (although I would probably use a name like `AssertSafeForLambdaCapture<T>`). I'm not the biggest fan of the workaround in general, but if you need to capture a raw pointer to a reference counted object by value it's probably the way to do it. This moves the decision to the place where the lambda is created which I think is much closer to the actual problem.
Attachment #8856132 - Flags: review?(michael) → review-
(Assignee)

Comment 61

4 months ago
mozreview-review-reply
Comment on attachment 8856132 [details]
Bug 1255894: Allow capturing `this` in lambda expressions when marked safe.

https://reviewboard.mozilla.org/r/128076/#review130936

I didn't particularly like the solution either. I'd rather make the decision based on the function we're passing the lambda to, but given how unlikely it is to be used elsewhere in the near future, I decided to go with the simpler solution.

I didn't realize the type attribute would be inherited by other class members, but given that it only allows `this` to be captured, I don't think it could have an effect on them.

The RawPtr solution doesn't help much, since we really need to capture and access `this`, and the SafeDispatch method was meant to avoid the noise of capturing both `this` and a separate RefPtr in so many different places. I wrote it before I realized the clang plugin would be a problem.

I'll try to come up with a better solution that works with the argument passed to the dispatch functions rather than the type.
(In reply to Kris Maglione [:kmag] from comment #61)
> The RawPtr solution doesn't help much, since we really need to capture and
> access `this`, and the SafeDispatch method was meant to avoid the noise of
> capturing both `this` and a separate RefPtr in so many different places. I
> wrote it before I realized the clang plugin would be a problem.

You don't have to capture both - you can just write:

    RefPtr<SelfType> self = this;
    auto lambda = [self] () { ...; self->PrivateMethod(); };

and it will work just fine. I much much prefer that you do that compared to this SafeDispatch method. We also support:

    RefPtr<SelfType> self = this;
    auto lambda = [this, self] () { SOME_MACRO_WHICH_EXPANDS_TO_CALL_A_METHOD_ON_THIS; };

and the static analysis recognizes that the `this` variable's capture is guarded. If you want to use `=` default captures, you can even do:

    RefPtr<SelfType> self = this;
    auto lambda = [=, self] () { /* You can use this in here and it will be safe! */ };

> I'll try to come up with a better solution that works with the argument
> passed to the dispatch functions rather than the type.

I would prefer that you capture the `this` pointer with a strong reference as referred to above. I don't like the idea of adding a new special case into the static analysis just to clean up a couple of lambda call sites which could easily be changed to obviously correctly keep their captures alive.
Comment on attachment 8850700 [details]
Bug 1255894: Part 4 - Implement StreamFilter IPC protocol.

https://reviewboard.mozilla.org/r/123236/#review134332

::: toolkit/components/extensions/webrequest/PStreamFilter.ipdl:9
(Diff revision 3)
>  
>  include protocol PBackground;
>  
>  namespace mozilla {
>  namespace extensions {
>  

We need a comment here describing what this protocol is for.

::: toolkit/components/extensions/webrequest/StreamFilterBase.h:22
(Diff revision 3)
> +{
> +public:
> +  typedef nsTArray<uint8_t> Data;
> +
> +protected:
> +  class BufferedData : public LinkedListElement<BufferedData> {

{ in a new line

::: toolkit/components/extensions/webrequest/StreamFilterBase.h:22
(Diff revision 3)
> +{
> +public:
> +  typedef nsTArray<uint8_t> Data;
> +
> +protected:
> +  class BufferedData : public LinkedListElement<BufferedData> {

final

::: toolkit/components/extensions/webrequest/StreamFilterChild.cpp:33
(Diff revision 3)
> +  case State::Disconnecting:
> +  case State::Disconnected:
> +    break;
> +
> +  default:
> +    ErrorResult rv;

IgnoredErrorResult

::: toolkit/components/extensions/webrequest/StreamFilterChild.cpp:427
(Diff revision 3)
> + * Glue
> + *****************************************************************************/
> +
>  void
>  StreamFilterChild::ActorDestroy(ActorDestroyReason aWhy)
>  {

you should have a similar policy as the parent actor here. You should call Cleanup here.

::: toolkit/components/extensions/webrequest/StreamFilterParent.h:131
(Diff revision 3)
> +    MOZ_ASSERT(NS_IsMainThread());
> +  }
> +
> +  // Ensures that a strong reference to this instance is held alive while a
> +  // method-based or closure-based runnable is being dispatched.
> +  class KungFuDeathGrip : public Runnable

final

::: toolkit/components/extensions/webrequest/StreamFilterParent.h:131
(Diff revision 3)
> +    MOZ_ASSERT(NS_IsMainThread());
> +  }
> +
> +  // Ensures that a strong reference to this instance is held alive while a
> +  // method-based or closure-based runnable is being dispatched.
> +  class KungFuDeathGrip : public Runnable

This name is super confusing. KungFuDeathGrip is not used for runnable names.
KeepAliveStreamFilterActorRunnable? Find a better name please.

::: toolkit/components/extensions/webrequest/StreamFilterParent.cpp:24
(Diff revision 3)
>  
> -using namespace mozilla::dom;
> +/*****************************************************************************
> + * Helpers
> + *****************************************************************************/
> +
> +StreamFilterParent::KungFuDeathGrip::KungFuDeathGrip(StreamFilterParent* aGrip,

Why this cannot be inline?

::: toolkit/components/extensions/webrequest/StreamFilterParent.cpp:31
(Diff revision 3)
> +  : mGrip(aGrip)
> +  , mRunnable(aRunnable)
> +{}
> +
> +NS_IMETHODIMP
> +StreamFilterParent::KungFuDeathGrip::Run()

same here.

::: toolkit/components/extensions/webrequest/StreamFilterParent.cpp:251
(Diff revision 3)
> +void
> +StreamFilterParent::DoSendData(Data&& aData)
> +{
> +  AssertIsPBackgroundThread();
> +
> +  if (mState == State::TransferringData) {

?
Attachment #8850700 - Flags: review?(amarchesini) → review+
Comment on attachment 8850702 [details]
Bug 1255894: Part 6 - Implement StreamFilter DOM bindings.

https://reviewboard.mozilla.org/r/123240/#review134340

::: toolkit/components/extensions/webrequest/StreamFilter.h:27
(Diff revision 3)
> +
> +class StreamFilterChild;
> +
> +using namespace mozilla::dom;
> +
> +class StreamFilter : public DOMEventTargetHelper

final?

::: toolkit/components/extensions/webrequest/StreamFilter.h:41
(Diff revision 3)
> +  static already_AddRefed<StreamFilter>
> +  Create(GlobalObject& global,
> +         uint64_t aRequestId,
> +         const nsAString& aAddonId);
> +
> +  explicit StreamFilter(nsIGlobalObject* aParent,

explicit is not needed here.

::: toolkit/components/extensions/webrequest/StreamFilter.h:70
(Diff revision 3)
> +  }
> +
> +  void Suspend(ErrorResult& aRv)
> +  {
> +    if (mActor) {
> +      mActor->Suspend(aRv);

you are not including the actor header.
This works only because we unified c++ files.
Move these methods to the cpp file.

::: toolkit/components/extensions/webrequest/StreamFilter.h:103
(Diff revision 3)
> +    } else {
> +      aRv.Throw(NS_ERROR_NOT_INITIALIZED);
> +    }
> +  }
> +
> +  nsISupports* GetParentObject() const { return mParent; }

This is not needed. DOMEventTargetHelper does it for you.

::: toolkit/components/extensions/webrequest/StreamFilter.h:124
(Diff revision 3)
> +
> +private:
> +  void
> +  ConnectToPBackground();
> +
> +  nsCOMPtr<nsIGlobalObject> mParent;

you don't need this.

::: toolkit/components/extensions/webrequest/StreamFilter.cpp:38
(Diff revision 3)
> + * Initialization
> + *****************************************************************************/
> +
> +StreamFilter::StreamFilter(nsIGlobalObject* aParent,
> +                           uint64_t aRequestId,
> +                           const nsAString& aAddonId)

DOMEventTargetHelper(aParent)

::: toolkit/components/extensions/webrequest/StreamFilter.cpp:39
(Diff revision 3)
> + *****************************************************************************/
> +
> +StreamFilter::StreamFilter(nsIGlobalObject* aParent,
> +                           uint64_t aRequestId,
> +                           const nsAString& aAddonId)
> +  : mParent(aParent)

and remove this.

::: toolkit/components/extensions/webrequest/StreamFilter.cpp:139
(Diff revision 3)
> +    ok = ReadTypedArrayData(data, aData.GetAsUint8Array(), aRv);
> +  } else {
> +    MOZ_ASSERT_UNREACHABLE("Argument should be ArrayBuffer or Uint8Array");
> +    return;
> +  }
> +

Do we want to keep the actor alive if ok is false?

::: toolkit/components/extensions/webrequest/StreamFilter.cpp:175
(Diff revision 3)
> +  init.mBubbles = false;
> +  init.mCancelable = false;
> +
> +  auto buffer = ArrayBuffer::Create(cx, this, aData.Length(), aData.Elements());
> +  if (!buffer) {
> +    // TODO: There is no way to recover from this. This chunk of data is lost.

follow up?
If we are OOM I would not dispatch an event.

::: toolkit/components/extensions/webrequest/StreamFilter.cpp:222
(Diff revision 3)
> +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(StreamFilter)
> +  NS_INTERFACE_MAP_ENTRY(nsIIPCBackgroundChildCreateCallback)
> +NS_INTERFACE_MAP_END_INHERITING(DOMEventTargetHelper)
> +
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(StreamFilter)
> +  NS_IMPL_CYCLE_COLLECTION_UNLINK(mParent)

remove this

::: toolkit/components/extensions/webrequest/StreamFilter.cpp:226
(Diff revision 3)
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(StreamFilter)
> +  NS_IMPL_CYCLE_COLLECTION_UNLINK(mParent)
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_END
> +
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(StreamFilter, DOMEventTargetHelper)
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mParent)

remove this

::: toolkit/components/extensions/webrequest/StreamFilterChild.h:139
(Diff revision 3)
>  
>    State mState;
>    State mNextState;
>    bool mReceivedOnStop;
> +
> +  StreamFilter* mStreamFilter;

a comment about why we have this raw pointer. And what keeps alive what.

::: toolkit/components/extensions/webrequest/StreamFilterEvents.h:26
(Diff revision 3)
> +namespace extensions {
> +
> +using namespace JS;
> +using namespace mozilla::dom;
> +
> +class StreamFilterDataEvent : public Event

final?

::: toolkit/components/extensions/webrequest/StreamFilterEvents.h:57
(Diff revision 3)
> +  void GetData(JSContext* aCx, JS::MutableHandleObject aResult)
> +  {
> +    aResult.set(mData);
> +  }
> +
> +  virtual JSObject* WrapObjectInternal(JSContext* aCx,

no virtual here and in the DTOR
Attachment #8850702 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 65

4 months ago
mozreview-review-reply
Comment on attachment 8850702 [details]
Bug 1255894: Part 6 - Implement StreamFilter DOM bindings.

https://reviewboard.mozilla.org/r/123240/#review134340

> explicit is not needed here.

C++11 allows implicit construction with multiple arguments, so I'd rather keep the `explicit`.

> you are not including the actor header.
> This works only because we unified c++ files.
> Move these methods to the cpp file.

I'm including StreamFilterChild.h, where this class is defined. I'd rather keep these methods inline, since they're only called from a single place in StreamFilterBinding, and it would be a shame to add extra linkage/call overhead for functions as simple as this.

> Do we want to keep the actor alive if ok is false?

I guess not. We should probably close the channel with NS_ERROR_OUT_OF_MEMORY.

> follow up?
> If we are OOM I would not dispatch an event.

I think we should probably close the channel with NS_ERROR_OUT_OF_MEMORY and still dispatch the error event.
(Assignee)

Comment 66

4 months ago
mozreview-review-reply
Comment on attachment 8850700 [details]
Bug 1255894: Part 4 - Implement StreamFilter IPC protocol.

https://reviewboard.mozilla.org/r/123236/#review134332

> ?

Sorry, I should have added a comment here when I split up the patch. The next patch in the series adds the logic that deals with attaching and manipulating traceable channels. Here, it adds a call to send data to the filtered channel.
(Assignee)

Comment 67

4 months ago
Thanks for the reviews :)
(Assignee)

Comment 68

4 months ago
I ran two sets of talos tests against these patches. Both sets compare running a skeleton WebExtension with a request listener for all URLs against the same extension that also adds a trivial response body filter to each request.

These are the results for in-process extensions:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=96fd2eb71647f50c0bd082645ed3609f34dcfb82&newProject=try&newRevision=4f727a99b3b767235187de53658623a3212f5265&framework=1&showOnlyImportant=0

And for out-of-process extensions:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=6c3f7be80481d153f06792d6ba56051e8b80edb3&newProject=try&newRevision=0abb3d405985020a1fb12865ab48989ac2ea44e7&framework=1&showOnlyImportant=0

In both cases, there is virtually no performance difference between the normal case and the request filtering case. In a few tests, there appear to be slight regressions (~.5-1ms on tp5o responsiveness), although the numbers for those are quite noisy, and within the margins of error.

The notable exceptions are the main netio metrics, which in almost all cases significantly decreased, and nonmain netio, which slightly increased. I'm not quite sure of the reason for this. My best guess is that it has something to do with redirecting the content conversions to the parent process, but I really don't know.

Comment 69

4 months ago
It seems that responses grabbed from the RAM cache cannot be read or altered by the current patchset; filter.onerror is triggered instead of ondata/onstop, where the event.target is a StreamFilter object with error "Invalid request ID" and status "failed" (and timestamp 0).

Comment 70

3 months ago
I have some related questions:
(Q1) Can the webRequest.onResponseData event be fired before the webRequest.onHeadersReceived event listeners are called?
(Q2) Can the webRequest.onResponseData event be fired before the webRequest.onHeadersReceived event listeners have returned?

To elaborate,

(Q1) If so, what guarantees that no onResponseData events are not dropped, because the following seems possible in theory:
1. (parent process->child process) webRequest.onHeadersReceived should be fired
2. (parent process) webRequest.onResponseData should be fired. There are no listeners yet, so the event is dropped.
3. (child process) webRequest.onHeadersReceived is fired.

(Q2) If not, how can an add-on implement redirect or cancel the request based on the response body?
For example, to implement content-sniffing if desired (e.g. if Content-Type is application/octet-stream and the response body starts with "%PDF", then the response is likely a PDF file).
Flags: needinfo?(g.maone)
We had a long discussion about this. Discussion was about how to implement this.

nsITracableChannel is tricky to make it work correctly in e10s. The problem is decision whether to decode data is made on the child process and in some cases also needs actual data. So we must fire OnDataAvailably (and deliver it from a parent to a child) before we know whether we should decode data or not. (nsITracableChannel should deliver decoded data).

From our discussion it seems that we can deliver data from one child(content process) to webextension process. This will make this much easier.
(Reporter)

Comment 72

3 months ago
(In reply to Rob Wu [:robwu] from comment #70)
> I have some related questions:
> (Q1) Can the webRequest.onResponseData event be fired before the
> webRequest.onHeadersReceived event listeners are called?
> (Q2) Can the webRequest.onResponseData event be fired before the
> webRequest.onHeadersReceived event listeners have returned?

If there was such an event I've been tempted to say "No" to both the questions, for the mechanism to be fully effective and flexible.
But as far as I can see from the patches the API is quite different now from the original suggestion: for instance, there's no onResponseData event but we've got a filterResponseData(requestId) function instead.
So I'll let Kris explain when/where it's supposed to be called and how it works.
Flags: needinfo?(g.maone) → needinfo?(kmaglione+bmo)
Keywords: dev-doc-needed

Comment 73

2 months ago
All, 

I am trying to to understand with all the changes if we will still be able to edit the content (JS/HTML) before it is rendered?

Thanks,
Michael
(In reply to Michael Balazs from comment #73)
> All, 
> 
> I am trying to to understand with all the changes if we will still be able
> to edit the content (JS/HTML) before it is rendered?
> 
> Thanks,
> Michael

Yes.

Comment 75

2 months ago
Is this code out in any release at the moment that we can begin testing with?

Thanks,
Michael
Hi Kris,

I will implement a part of this bug, or I can take over the bug, as you prefer. I just need couple of clarifications on how webextensions work:

I have looked at your patches and most of StreamFilterParent/Child can move as it is to be between Child and WebExtension process.

The only problematic thing is WebRequest.jsm which keeps track of permissions. Is this correct? And it is sitting on the parent process?

HttpChannelChild needs to implement nsITraceableChannel which is easy.

when do we want to allow registration of a streamFilter? before OnStartRequest or also after OnStartRequest?
If we want to allow it after OnStartRequest it is a bit tricky to install the the listener on the right place, i.e. after decoders.
I tried to finish this bug. I rebased the patches but I have problem with the test.
I see an error:
JavaScript error: resource://gre/modules/ExtensionCommon.jsm, line 435: Error: Not implemented

but I really do not know anything about webextensions and I do not know how to fix it.

the error occurs when filterResponseData is called. I have change ext-c-webRequest.js to:

"use strict";

this.webRequest = class extends ExtensionAPI {
  getAPI(context) {
    return {
      webRequest: {
        filterResponseData(requestId) {
          requestId = parseInt(requestId, 10);

          return context.cloneScope.StreamFilter.create(
            requestId, context.extension.id);
        },
      },
    };
  }
};
With robwu_nl's help, Ihave fix the problem from comment 77.
While working on this, I realized one potential problem.

I am working on integrating comment 71. All data will be sent from a content process. If I remember correctly other events (on-request-sent, I do not know if on-response is used) will be sent from the parent process. It will be hard to guarantee synchronization of this events.

Could this be a potential problem? :mao, do you know answer to this question or do you know who knows?
Flags: needinfo?(g.maone)
(Reporter)

Comment 80

2 months ago
(In reply to Dragana Damjanovic [:dragana] from comment #79)
> All data will be sent from a content
> process. If I remember correctly other events (on-request-sent, I do not
> know if on-response is used) will be sent from the parent process. It will
> be hard to guarantee synchronization of this events.
> 
> Could this be a potential problem? :mao, do you know answer to this question
> or do you know who knows?

The main if not the only problem I could see (beside properly documenting this async behavior): given this limitation will we be able to access the response headers (and especially content-type) from within our filter?
Flags: needinfo?(g.maone)

Comment 81

22 days ago
mozreview-review
Comment on attachment 8850701 [details]
Bug 1255894: Part 5 - Hook up StreamFilterParent as a stream listener.

https://reviewboard.mozilla.org/r/123238/#review167676

Dropping my review on this one. This needs to be done in a different way, as we discussed a couple of weeks ago.
Attachment #8850701 - Flags: review?(dd.mozilla)
You need to log in before you can comment on or make changes to this bug.