Closed Bug 1405286 Opened 2 years ago Closed 2 years ago

Data written by StreamFilter.write appears out of order

Categories

(WebExtensions :: Request Handling, defect, P3)

57 Branch
defect

Tracking

(firefox57 fixed, firefox58 fixed)

RESOLVED FIXED
mozilla58
Tracking Status
firefox57 --- fixed
firefox58 --- fixed

People

(Reporter: robwu, Assigned: kmag)

References

Details

Attachments

(5 files)

Attached file webreqoutoforder.xpi
STR:

1. Load the attached extension.
2. Visit any website, e.g. example.com.
   (The extension will use webRequest.filterResponseData to rewrite the stream, and upon filter.onstart call filter.write 1000 times).
3. If the response seems to look good, reload the page.

Expected:
- Even after 10 times of reloading, the response should still consistst of "This is a response at <timestamp>" and "Data <n>" repeated 999 times, in order.

Actual:
- Sometimes the data is rendered out of order. This seems to occur more often if the number of filter.write() calls increases.

For example, when I ran the extension (with 100x instead of 1000x write()) for a txt file on localhost (no cache), the response looks as follows. In this example, it seems that the write() call is handled by two threads that run in parallel.

Data 7 <br>
... cut data 8 until 27 ...
Data 28 <br>
This is a response at 1507034804737 <br>
Data 29 <br>
Data 30 <br>
Data 1 <br>
Data 31 <br>
Data 2 <br>
Data 32 <br>
Data 33 <br>
Data 3 <br>
Data 34 <br>
Data 4 <br>
Data 35 <br>
Data 5 <br>
Data 36 <br>
... cut data 37 until 51 ...
Data 52 <br>
Data 6 <br>
Data 53 <br>
... cut data 54 until 98 ...
Data 99 <br>
Priority: -- → P3
Blocks: 1380255
Comment on attachment 8918609 [details]
Bug 1405286: Part 2 - Ensure ordered processing of StreamFilter events.

https://reviewboard.mozilla.org/r/189454/#review194652


C/C++ static analysis found 3 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/extensions/webrequest/StreamFilterParent.cpp:52
(Diff revision 1)
> +  {
> +    return do_AddRef(mTarget.get());
> +  }
> +
> +protected:
> +  virtual ~ChannelEventWrapper() = default;

Warning: Prefer using 'override' or (rarely) 'final' instead of 'virtual' [clang-tidy: modernize-use-override]

  virtual ~ChannelEventWrapper() = default;
          ^
                                 override 

::: toolkit/components/extensions/webrequest/StreamFilterParent.cpp:72
(Diff revision 1)
> +  {
> +    mFunc();
> +  }
> +
> +protected:
> +  virtual ~ChannelEventFunction() = default;

Warning: Prefer using 'override' or (rarely) 'final' instead of 'virtual' [clang-tidy: modernize-use-override]

  virtual ~ChannelEventFunction() = default;
          ^
                                  override 

::: toolkit/components/extensions/webrequest/StreamFilterParent.cpp:93
(Diff revision 1)
> +    nsresult rv = mRunnable->Run();
> +    Unused << NS_WARN_IF(NS_FAILED(rv));
> +  }
> +
> +protected:
> +  virtual ~ChannelEventRunnable() = default;

Warning: Prefer using 'override' or (rarely) 'final' instead of 'virtual' [clang-tidy: modernize-use-override]

  virtual ~ChannelEventRunnable() = default;
          ^
                                  override
Comment on attachment 8918608 [details]
Bug 1405286: Part 1 - Allow retrieving the delivery target from retargetable requests.

https://reviewboard.mozilla.org/r/189452/#review194800

::: netwerk/base/nsIThreadRetargetableRequest.idl:36
(Diff revision 1)
>     * should be ready to deal with OnDataAvailable on either the main thread or
>     * the new target thread.
>     */
>    void retargetDeliveryTo(in nsIEventTarget aNewTarget);
> +
> +  readonly attribute nsIEventTarget deliveryTarget;

Please write some comment.
And please write that this is available only after ONStartRequest is called.
Attachment #8918608 - Flags: review?(dd.mozilla) → review+
Comment on attachment 8918610 [details]
Bug 1405286: Part 3 - Test that filterResponseData from cached onHeadersReceived doesn't crash.

https://reviewboard.mozilla.org/r/189456/#review195008
Attachment #8918610 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8918611 [details]
Bug 1405286: Part 4 - Don't overwrite existing state with finishedtransferringdata.

https://reviewboard.mozilla.org/r/189458/#review195012
Attachment #8918611 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8918608 [details]
Bug 1405286: Part 1 - Allow retrieving the delivery target from retargetable requests.

https://reviewboard.mozilla.org/r/189452/#review194800

> Please write some comment.
> And please write that this is available only after ONStartRequest is called.

Oops. I meant to add a comment before I committed.
Comment on attachment 8918609 [details]
Bug 1405286: Part 2 - Ensure ordered processing of StreamFilter events.

https://reviewboard.mozilla.org/r/189454/#review195054

I'm not super comfortable with the event/queue stuff since I've never looked at it.  Just spent the time doing so, and everything looks fine.
Attachment #8918609 - Flags: review?(mixedpuppy) → review+
Assignee: nobody → kmaglione+bmo
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d7c80924f6059d29bb9571220e4a09d4d74a319
Bug 1405286: Part 1 - Allow retrieving the delivery target from retargetable requests. r=dragana

https://hg.mozilla.org/integration/mozilla-inbound/rev/05d11d7b50944b99db6e06a1124ebdebe2510648
Bug 1405286: Part 2 - Ensure ordered processing of StreamFilter events. r=mixedpuppy

https://hg.mozilla.org/integration/mozilla-inbound/rev/fede70f8a899eaf7e546d6ed6d5d31fe725f52a7
Bug 1405286: Part 3 - Test that filterResponseData from cached onHeadersReceived doesn't crash. r=mixedpuppy

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ded456b65770dcfa0e4fc7d63d81a826d50702d
Bug 1405286: Part 4 - Don't overwrite existing state with finishedtransferringdata. r=mixedpuppy
Comment on attachment 8918609 [details]
Bug 1405286: Part 2 - Ensure ordered processing of StreamFilter events.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1255894
[User impact if declined]: This issue causes chunks of HTTP responses to be processed out of order in some circumstances when extensions are filtering response data, which can lead to arbitrary, strange breakage across the web.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: No.
[Needs manual test from QE? If yes, steps to reproduce]: No, automated tests should be sufficient.
[List of other uplifts needed for the feature/fix]: Only Part 1 from this bug.
[Is the change risky?]: Somewhat, but less risky than not uplifting it.
[Why is the change risky/not risky?]: The patch changes some of the core event dispatching code for the StreamFilter API, and the logic is somewhat subtle. However, these changes only take effect when the StreamFilter API is in use, and the behavior without these changes is more problematic than any subtle bugs that are likely to arise as their result.
[String changes made/needed]: None.
Attachment #8918609 - Flags: approval-mozilla-beta?
Comment on attachment 8918609 [details]
Bug 1405286: Part 2 - Ensure ordered processing of StreamFilter events.

Despite the risk associated, I am told this is a must fix, Beta57+
Attachment #8918609 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Blocks: 1405375
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #13)
> [Is this code covered by automated tests?]: Yes.
> [Has the fix been verified in Nightly?]: No.
> [Needs manual test from QE? If yes, steps to reproduce]: No, automated tests
> should be sufficient.

Setting qe-verify- based on Kris's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.