1192333 - Use channel->ascynOpen2 in dom/xslt/xslt/txMozillaStylesheetCompiler.cpp

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1192333_txmozillastylesheetcompiler.patch

Jonas, please note that in both cases (and also within nsSyncLoadService patch - see Bug 1191645) the contentPolicy check uses TYPE_STYLESHEET but the actual channel is created using TYPE_XSLT. I think we should keep TYPE_XSLT (also because of the mimetype guess).

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8645095 [details] [diff] [review]
bug_1192333_txmozillastylesheetcompiler.patch

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

::: dom/xslt/xslt/txMozillaStylesheetCompiler.cpp
@@ -507,5 @@
> -        new nsCORSListenerProxy(sink, aReferrerPrincipal, false);
> -    rv = listener->Init(channel, DataURIHandling::Disallow);
> -    NS_ENSURE_SUCCESS(rv, rv);
> -
> -    return channel->AsyncOpen(listener, parser);

You need to change the sink to not rely on the aContext argument. I.e. make the sink hold on to an mParser member, and change s/aContext/mParser/ in OnStartRequest/OnStopRequest/OnDataAvailable.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1192333_txmozillastylesheetcompiler.patch

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=924ac30fd23d

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1192333_txmozillastylesheetcompiler_mem_leak_fix.patch

Jonas, when pushing to TRY recently [1] I realized we do have a memory leak. I can locally reproduce when running:
> ./mach mochitest dom/xslt/tests/mochitest/

The attached patch fixes the problem. I assume it is the right change - can you confirm?

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=df783af9d0d8

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1192333_txmozillastylesheetcompiler.patch

Folded the two patches into one. Carrying over r+ from Jonas.

Let's make sure everything is fine now:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a6aafa3e0fe

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1192333_txmozillastylesheetcompiler_web_platform_tests.patch

I am not exactly sure how our patch is related to that web platform test but it looks like it fixes the test on e10s. Maybe a timing issue we fixed? Probably, because the commit message when disabling the tests was:
> Bug 1203906 - Disable some unstable wpt tests when running with e10s enabled, a=testonly

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8665500 [details] [diff] [review]
bug_1192333_txmozillastylesheetcompiler_web_platform_tests.patch

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

I don't believe that any XSLT changes could fix these tests. Something else is going on here.

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jonas Sicking (:sicking) from comment #8)
> Comment on attachment 8665500 [details] [diff] [review]
> bug_1192333_txmozillastylesheetcompiler_web_platform_tests.patch
> 
> Review of attachment 8665500 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't believe that any XSLT changes could fix these tests. Something else
> is going on here.

Ehsan, I think Ben is on PTO, any ideas what might cause these tests to pass?

Comment 10

[(no longer active)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #9)
> (In reply to Jonas Sicking (:sicking) from comment #8)
> > Comment on attachment 8665500 [details] [diff] [review]
> > bug_1192333_txmozillastylesheetcompiler_web_platform_tests.patch
> > 
> > Review of attachment 8665500 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I don't believe that any XSLT changes could fix these tests. Something else
> > is going on here.
> 
> Ehsan, I think Ben is on PTO, any ideas what might cause these tests to pass?

I saw these today on another unrelated try push: <https://treeherder.mozilla.org/#/jobs?repo=try&revision=42525b835e39>  And Michael also told me on IRC that he is seeing them on another unrelated push.  Maybe James can explain what's going on?  The only thing that jumps at me is that using OS version numbers like this is super fragile...

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Ehsan Akhgari (don't ask for review please) from comment #10)
> I saw these today on another unrelated try push:
> <https://treeherder.mozilla.org/#/jobs?repo=try&revision=42525b835e39>  And
> Michael also told me on IRC that he is seeing them on another unrelated
> push.  Maybe James can explain what's going on?  The only thing that jumps
> at me is that using OS version numbers like this is super fragile...

And yet another one over in Bug 1199295:
> https://treeherder.mozilla.org/#/jobs?repo=try&tochange=560176ee021f

Comment 12

[James Graham [:jgraham]]

I think it's a perma-orange on try that occurred because someone landed a fix without updating the metadata whilst these jobs were hidden on try. It will go away when I land a wpt update which should be today (although Windows builds are super-slow at the moment, so that might be optimistic).

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8665500 [details] [diff] [review]
bug_1192333_txmozillastylesheetcompiler_web_platform_tests.patch

(In reply to James Graham [:jgraham] from comment #12)
> I think it's a perma-orange on try that occurred because someone landed a
> fix without updating the metadata whilst these jobs were hidden on try. It
> will go away when I land a wpt update which should be today (although
> Windows builds are super-slow at the moment, so that might be optimistic).

Sounds good to me. Marking that patch as obsolete then and try to land early next week. Thanks for the info!

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

(In reply to James Graham [:jgraham] from comment #12)
> I think it's a perma-orange on try that occurred because someone landed a
> fix without updating the metadata whilst these jobs were hidden on try. It
> will go away when I land a wpt update which should be today (although
> Windows builds are super-slow at the moment, so that might be optimistic).

James, can please let me know once the problem is fixed so I can land my patches?

Comment 15

[James Graham [:jgraham]]

wpt-e10s isn't yet enabled on inbound so if that's the only orange you see you are free to land it whenever. But I'm working on the issue as we speak.

Comment 16

[Pulsebot]

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

Comment 17

[Carsten Book [:Tomcat]]

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