New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix brotli decoding #22616

Merged
merged 1 commit into from Jan 15, 2019

Conversation

Projects
None yet
7 participants
@jdm
Copy link
Member

jdm commented Jan 4, 2019

This replaces our current decoding setup by https://github.com/seanmonstar/reqwest/blob/master/src/async_impl/decoder.rs, and integrates brotli and deflate decoding to maintain our existing support.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #22228
  • There are tests for these changes

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

highfive commented Jan 4, 2019

Heads up! This PR modifies the following files:

  • @KiChjang: components/net/Cargo.toml, components/net/lib.rs, components/net/http_loader.rs, components/net/decoder.rs, components/net/connector.rs
@highfive

This comment has been minimized.

Copy link

highfive commented Jan 4, 2019

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@jdm

This comment has been minimized.

Copy link
Member Author

jdm commented Jan 4, 2019

Waiting to hear about dropbox/rust-brotli-decompressor#4 to see if we need to maintain the brotli fork long-term.

@jdm

This comment has been minimized.

Copy link
Member Author

jdm commented Jan 4, 2019

@bors-servo try=wpt

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 4, 2019

⌛️ Trying commit 4542c80 with merge 7c4f065...

bors-servo added a commit that referenced this pull request Jan 4, 2019

Auto merge of #22616 - jdm:google-decode, r=<try>
Fix brotli decoding work

This replaces our current decoding setup by https://github.com/seanmonstar/reqwest/blob/master/src/async_impl/decoder.rs, and integrates brotli and deflate decoding to maintain our existing support.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #22228
- [x] There are tests for these changes

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22616)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 4, 2019

💔 Test failed - linux-rel-wpt

@jdm jdm changed the title Fix brotli decoding work Fix brotli decoding Jan 4, 2019


impl fmt::Debug for Decoder {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
f.debug_struct("Decoder").finish()

This comment has been minimized.

@Eijebong

Eijebong Jan 4, 2019

Member

Not sure what this is exactly doing but I don't think it gives the type of decode and it might be useful to know it ? i.e plain, gzip...

@jdm jdm force-pushed the jdm:google-decode branch from 4542c80 to 58036d4 Jan 4, 2019

@highfive highfive removed the S-tests-failed label Jan 4, 2019

@jdm jdm force-pushed the jdm:google-decode branch 3 times, most recently from f7b5b42 to 76dd959 Jan 4, 2019

@jdm

This comment has been minimized.

Copy link
Member Author

jdm commented Jan 4, 2019

@bors-servo try=wpt

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 4, 2019

⌛️ Trying commit 76dd959 with merge 909ef89...

bors-servo added a commit that referenced this pull request Jan 4, 2019

Auto merge of #22616 - jdm:google-decode, r=<try>
Fix brotli decoding

This replaces our current decoding setup by https://github.com/seanmonstar/reqwest/blob/master/src/async_impl/decoder.rs, and integrates brotli and deflate decoding to maintain our existing support.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #22228
- [x] There are tests for these changes

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22616)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 4, 2019

💔 Test failed - linux-rel-wpt

@nox
Copy link
Member

nox left a comment

The failures seem relevant to the patch, do you want me to look into them?

var input = client.responseText;
var client2 = new XMLHttpRequest()
client2.open("GET", "resources/brotli.py?pipe=trickle(1:d1)", false);
client2.onload = test.step_func(function() {

This comment has been minimized.

@nox

nox Jan 8, 2019

Member

Nit: this can be step_func_done.

assert_equals(client.responseText, input);
test.done();
}
async_test(function(test) {

This comment has been minimized.

@nox

nox Jan 8, 2019

Member

Nit: you could do var test = async_test(); and not have to indent the entire test.

This comment has been minimized.

@jdm

jdm Jan 9, 2019

Author Member

That's how the test was originally written, but that excludes the code outside of the test from reporting errors. This format provides better coverage.

@nox nox assigned nox and unassigned asajeffrey Jan 8, 2019

@jdm jdm force-pushed the jdm:google-decode branch from 76dd959 to 6e29bdc Jan 9, 2019

@highfive highfive removed the S-tests-failed label Jan 9, 2019

@jdm

This comment has been minimized.

Copy link
Member Author

jdm commented Jan 9, 2019

@bors-servo r=nox

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 9, 2019

📌 Commit 6e29bdc has been approved by nox

@jdm jdm removed the S-awaiting-review label Jan 15, 2019

@jdm

This comment has been minimized.

Copy link
Member Author

jdm commented Jan 15, 2019

@bors-servo r=nox

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 15, 2019

📌 Commit 047a6b3 has been approved by nox

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 15, 2019

⌛️ Testing commit 047a6b3 with merge bc3832f...

bors-servo added a commit that referenced this pull request Jan 15, 2019

Auto merge of #22616 - jdm:google-decode, r=nox
Fix brotli decoding

This replaces our current decoding setup by https://github.com/seanmonstar/reqwest/blob/master/src/async_impl/decoder.rs, and integrates brotli and deflate decoding to maintain our existing support.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #22228
- [x] There are tests for these changes

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22616)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 15, 2019

💔 Test failed - status-taskcluster

@CYBAI

This comment has been minimized.

Copy link
Collaborator

CYBAI commented Jan 15, 2019

Both task cluster and Travis failed with following error 👀

+ ./mach test-tidy --no-progress --all
Error running mach:

    ['test-tidy', '--no-progress', '--all']

The error occurred in code that was called by the mach command. This is either
a bug in the called code itself or in the way that mach is calling it.

You should consider filing a bug for this issue.

If filing a bug, please include the full output of mach, including this error
message.

The details of the failure are as follows:

AttributeError: 'NoneType' object has no attribute 'to_json'

  File "/repo/python/servo/testing_commands.py", line 330, in test_tidy
    manifest_dirty = run_update(self.context.topdir, check_clean=True)
  File "/repo/python/servo/testing_commands.py", line 90, in run_update
    return manifestupdate.update(logger, wpt_dir, check_clean, rebuild)
  File "/repo/tests/wpt/manifestupdate.py", line 54, in update
    return _check_clean(logger, test_paths)
  File "/repo/tests/wpt/manifestupdate.py", line 81, in _check_clean
    old_manifest.to_json())
@jdm

This comment has been minimized.

Copy link
Member Author

jdm commented Jan 15, 2019

sounds of wailing and gnashing of teeth

@jdm jdm force-pushed the jdm:google-decode branch from 047a6b3 to 6404a0e Jan 15, 2019

@jdm

This comment has been minimized.

Copy link
Member Author

jdm commented Jan 15, 2019

@bors-servo r=nox

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 15, 2019

📌 Commit 6404a0e has been approved by nox

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 15, 2019

⌛️ Testing commit 6404a0e with merge 4f45eea...

bors-servo added a commit that referenced this pull request Jan 15, 2019

Auto merge of #22616 - jdm:google-decode, r=nox
Fix brotli decoding

This replaces our current decoding setup by https://github.com/seanmonstar/reqwest/blob/master/src/async_impl/decoder.rs, and integrates brotli and deflate decoding to maintain our existing support.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #22228
- [x] There are tests for these changes

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22616)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 15, 2019

💔 Test failed - linux-rel-css

@jdm

This comment has been minimized.

Copy link
Member Author

jdm commented Jan 15, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 15, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 15, 2019

@bors-servo bors-servo merged commit 6404a0e into servo:master Jan 15, 2019

3 checks passed

Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment