Redirected HTTP channel doesn't inherit the channel priority

RESOLVED FIXED in Firefox 54

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: schien, Assigned: schien)

Tracking

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: [necko-active])

Attachments

(2 attachments)

The redirected HTTP channel should inherit the channel priority to preserve the loading order.

Maybe we can set priority in here:
https://searchfox.org/mozilla-central/rev/848c29538ab007fb95dc6cff194f0e3d3809613d/netwerk/protocol/http/HttpBaseChannel.cpp#3012
Good catch!  Yes, that is the place.
Assignee: nobody → schien
Whiteboard: [necko-active]
Comment on attachment 8835932 [details]
Bug 1337721 - Part 1, preserve the channel priority after redirect.

https://reviewboard.mozilla.org/r/111486/#review112834

thanks!
Attachment #8835932 - Flags: review?(honzab.moz) → review+
Comment on attachment 8835933 [details]
Bug 1337721 - Part 2, add xpcshell test for channel priority.

https://reviewboard.mozilla.org/r/111488/#review112842

::: netwerk/test/unit/test_channel_priority.js:52
(Diff revision 1)
> +  requestChannel.asyncOpen2(new ChannelListener(checkResponse, requestChannel));
> +}
> +
> +function checkResponse(request, buffer, requestChannel) {
> +  requestChannel.QueryInterface(Ci.nsISupportsPriority);
> +  Assert.equal(requestChannel.priority, Ci.nsISupportsPriority.PRIORITY_HIGHEST);

what is the requestChannel?

::: netwerk/test/unit/test_channel_priority.js:73
(Diff revision 1)
> +
> +function run_test() { // jshint ignore:line
> +  if (!runningInParent) {
> +    // report test finished to parent process
> +    add_test(function () {
> +      do_send_remote_message('finished');

are you sure this is the place to send 'finish'?  shouldn't it happen after all the requests are made?
Comment on attachment 8835933 [details]
Bug 1337721 - Part 2, add xpcshell test for channel priority.

https://reviewboard.mozilla.org/r/111488/#review112842

> what is the requestChannel?

This is the original channel object we created in line 44. We store it as the closure context for the `checkResponse` callback.
See https://searchfox.org/mozilla-central/rev/d3307f19d5dac31d7d36fc206b00b686de82eee4/netwerk/test/unit/head_channels.js#51 and https://searchfox.org/mozilla-central/rev/d3307f19d5dac31d7d36fc206b00b686de82eee4/netwerk/test/unit/head_channels.js#170

> are you sure this is the place to send 'finish'?  shouldn't it happen after all the requests are made?

This code will only send 'finish' after all the requests are made because it is added as the last test step.
I should change the comment to:
```
// add a task to report test finished to parent process at the end of test queue,
// since do_register_cleanup is not available in child xpcshell test script.
```
Comment on attachment 8835933 [details]
Bug 1337721 - Part 2, add xpcshell test for channel priority.

https://reviewboard.mozilla.org/r/111488/#review115232
Comment on attachment 8835933 [details]
Bug 1337721 - Part 2, add xpcshell test for channel priority.

stupid reviewberd
Attachment #8835933 - Flags: review?(honzab.moz) → review+
hmm...the r+ flag didn't reflect to reviewboard. Let me update the r+ on reviewboard manually.
Comment on attachment 8835933 [details]
Bug 1337721 - Part 2, add xpcshell test for channel priority.

https://reviewboard.mozilla.org/r/111488/#review115504

help reflect the r+ flag from bugzilla.
Attachment #8835933 - Flags: review+
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #12)
> Comment on attachment 8835933 [details]
> Bug 1337721 - Part 2, add xpcshell test for channel priority.
> 
> https://reviewboard.mozilla.org/r/111488/#review115504
> 
> help reflect the r+ flag from bugzilla.

Hmm...the commit message of part-2 will be rewrite to "r=schien" because I click r+ on reviewboard. Still need @mayhemer's r+ on reviewboard to avoid the confusion in commit message.
Flags: needinfo?(honzab.moz)
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #13)
> (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment
> #12)
> > Comment on attachment 8835933 [details]
> > Bug 1337721 - Part 2, add xpcshell test for channel priority.
> > 
> > https://reviewboard.mozilla.org/r/111488/#review115504
> > 
> > help reflect the r+ flag from bugzilla.
> 
> Hmm...the commit message of part-2 will be rewrite to "r=schien" because I
> click r+ on reviewboard. Still need @mayhemer's r+ on reviewboard to avoid
> the confusion in commit message.

There is one.  Easy solution: don't use crappy reviewboard :)
Flags: needinfo?(honzab.moz)
Comment on attachment 8835933 [details]
Bug 1337721 - Part 2, add xpcshell test for channel priority.

https://reviewboard.mozilla.org/r/111488/#review115538
Pushed by schien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b2ba4b8eb7f3
Part 1, preserve the channel priority after redirect. r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/f7fd37690c35
Part 2, add xpcshell test for channel priority. r=mayhemer
https://hg.mozilla.org/mozilla-central/rev/b2ba4b8eb7f3
https://hg.mozilla.org/mozilla-central/rev/f7fd37690c35
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.