Closed Bug 1234757 Opened 4 years ago Closed 4 years ago

Use channel.asyncOpen2 within netwerk/test/unit/test_protocolproxyservice.js

Categories

(Core :: DOM: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: ckerschb, Assigned: mcmanus)

References

Details

Attachments

(2 files, 1 obsolete file)

No description provided.
Assignee: nobody → mozilla
Blocks: 1193558
Status: NEW → ASSIGNED
That patch was already r+ within 1234596 by Patrick but got backed out because the test timed out. Let's investigate within this bug what's going on and why this test was failing.
Attachment #8701340 - Flags: review+
Hey Patrick, this patch got backed out and finally I got a chance to investigate. It's only one specific subtest where the second arg of .AsyncOpen() was used for the context.

In the enclosed patch, if you grep for 'AsyncOpen2' you'll see what I mean. I am not sure how to fix. Question is, do we need to keep that subtest? If so, should we basically create some kind of mediator pattern to forward the context through the directFilterListener? We have done that for 2 or 3 callistes within C++ so far, not sure if it makes sense to do that for the test though. Please let me know what you think - thanks!
Attachment #8701340 - Attachment is obsolete: true
Flags: needinfo?(mcmanus)
Assignee: mozilla → mcmanus
that subtest doesn't really need the second argument.. the patch should fix that up and then you can rebase and apply your patch to make it use asyncopen2().

thanks!
Flags: needinfo?(mcmanus)
Comment on attachment 8708432 [details] [diff] [review]
test_protocolproxyservice does not need 2nd arg to asyncopen

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

::: netwerk/test/unit/test_protocolproxyservice.js
@@ +992,5 @@
>                               Services.scriptSecurityManager.getSystemPrincipal(),
>                               null,      // aTriggeringPrincipal
>                               Ci.nsILoadInfo.SEC_NORMAL,
>                               Ci.nsIContentPolicy.TYPE_OTHER);
> +  chan.asyncOpen(directFilterListener, null);

This should be asyncOpen2, or do you wanna keep that test around?
Attachment #8708432 - Flags: review?(mozilla)
Comment on attachment 8708432 [details] [diff] [review]
test_protocolproxyservice does not need 2nd arg to asyncopen

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

::: netwerk/test/unit/test_protocolproxyservice.js
@@ +992,5 @@
>                               Services.scriptSecurityManager.getSystemPrincipal(),
>                               null,      // aTriggeringPrincipal
>                               Ci.nsILoadInfo.SEC_NORMAL,
>                               Ci.nsIContentPolicy.TYPE_OTHER);
> +  chan.asyncOpen(directFilterListener, null);

I just meant this as a precursor to your asyncopen2() patch..
Comment on attachment 8708432 [details] [diff] [review]
test_protocolproxyservice does not need 2nd arg to asyncopen

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

(In reply to Patrick McManus [:mcmanus] from comment #4)
> that subtest doesn't really need the second argument.. the patch should fix
> that up and then you can rebase and apply your patch to make it use
> asyncopen2().
> 
> thanks!

Oh sorry, I haven't seen your text! Makes sense!
Attachment #8708432 - Flags: review+
Comment on attachment 8708135 [details] [diff] [review]
bug_1234757_asyncopen2_test_protocolproxyservice.patch

Carrying over r+ from Patrick!
Attachment #8708135 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/fdef354718b1
https://hg.mozilla.org/mozilla-central/rev/5ee1ef34168e
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.