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

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1234757_asyncopen2_test_protocolproxyservice.patch

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.

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1234757_asyncopen2_test_protocolproxyservice.patch

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!

Comment 3

[Patrick McManus [:mcmanus]]

Attachment: test_protocolproxyservice does not need 2nd arg to asyncopen

Comment 4

[Patrick McManus [: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!

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

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?

Comment 6

[Patrick McManus [: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);

I just meant this as a precursor to your asyncopen2() patch..

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

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!

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8708135 [details] [diff] [review]
bug_1234757_asyncopen2_test_protocolproxyservice.patch

Carrying over r+ from Patrick!

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fdef354718b1
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ee1ef34168e

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fdef354718b1
https://hg.mozilla.org/mozilla-central/rev/5ee1ef34168e