1242731 - Convert callsites to use channel.async0pen2() within browser/extensions/shumway

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1242731_asyncopen2_shumway.patch

Hey Yury, at some point we would need to use asyncOpen2() for shumway. Maybe you can help me answer those three questions to move forward with this bug:

1) Providing a WIP patch, but asyncOpen2() does not take a second argument anymore. Is it really needed in that case? Do we know that? In some cases where we switched to asyncOpen2() we where able to just remove the second argument because it was basically unused. In some other cases we created some kind of mediator pattern within the streamListener that forwards the context underneath.

2) Are there any tests which I can run to make sure shumway is not broken by my changes?

3) Can we land that patch on mozilla-central?

Comment 2

[Yury Delendik (:yury)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #1)
> Created attachment 8711877 [details] [diff] [review]
> bug_1242731_asyncopen2_shumway.patch
> 
> Hey Yury, at some point we would need to use asyncOpen2() for shumway. Maybe
> you can help me answer those three questions to move forward with this bug:
> 
> 1) Providing a WIP patch, but asyncOpen2() does not take a second argument
> anymore. Is it really needed in that case? Do we know that? In some cases
> where we switched to asyncOpen2() we where able to just remove the second
> argument because it was basically unused. In some other cases we created
> some kind of mediator pattern within the streamListener that forwards the
> context underneath.

The code for the proxy can be found above or at (https://github.com/mozilla/shumway/blob/master/extension/firefox/content/ShumwayStreamConverter.jsm#L260). Looks like context is not used, however it's passed to the underlined listener. So I'm not sure if stream converter underline listeners needs context.

> 2) Are there any tests which I can run to make sure shumway is not broken by
> my changes?

m-c has not such tests. You can manually test this functionality by turning off shumway.disabled, restart ff and check e.g. http://www.homestarrunner.com/sbemail41.html (context menu will indicate that Shumway is used).

> 3) Can we land that patch on mozilla-central?

Yes. Can you also open a ticket upstream so we will track this change there?

Comment 3

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1242731_asyncopen2_shumway.patch

(In reply to Yury Delendik (:yury) from comment #2)
> The code for the proxy can be found above or at
> (https://github.com/mozilla/shumway/blob/master/extension/firefox/content/
> ShumwayStreamConverter.jsm#L260). Looks like context is not used, however
> it's passed to the underlined listener. So I'm not sure if stream converter
> underline listeners needs context.

I am not sure myself, given that it works I suppose it's fine, but ultimately it's your call obviously.

> m-c has not such tests. You can manually test this functionality by turning
> off shumway.disabled, restart ff and check e.g.
> http://www.homestarrunner.com/sbemail41.html (context menu will indicate
> that Shumway is used).

I flipped the switch for shumway.disabled and loaded http://www.homestarrunner.com/sbemail41.html - no security errors and seems to work fine. However, I noticed (even without my patch applied) the following error which we potentially should fix.

Security Error: Content at moz-nullprincipal:{e2a46ddb-8b89-4d7b-8954-72c089faf7e3} may not load data from resource://shumway/lib/mp3/mp3worker.js.
 
> > 3) Can we land that patch on mozilla-central?
> 
> Yes. Can you also open a ticket upstream so we will track this change there?

Created a new issue: https://github.com/mozilla/shumway/issues/2410

Btw, the changes for NetUtil.newChannel()-API landed for FF38 (http://hg.mozilla.org/mozilla-central/rev/e6b4bb4cc393), not sure if we still need to create some kind of branch which we used to have before calling NewChannel2.

Comment 4

[Yury Delendik (:yury)]

Comment on attachment 8714458 [details] [diff] [review]
bug_1242731_asyncopen2_shumway.patch

LGTM

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef6c6b82b0fb

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ef6c6b82b0fb