Closed Bug 1192333 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: DOM: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: ckerschb, Assigned: ckerschb)

References

Details

Attachments

(1 file, 4 obsolete files)

No description provided.
Assignee: nobody → mozilla
Blocks: 1182535
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).
Attachment #8645095 - Flags: review?(jonas)
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.
Attachment #8645095 - Flags: review?(jonas) → review-
Attachment #8645095 - Attachment is obsolete: true
Attachment #8648284 - Flags: review?(jonas)
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
Attachment #8663884 - Flags: review?(jonas)
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
Attachment #8648284 - Attachment is obsolete: true
Attachment #8663884 - Attachment is obsolete: true
Attachment #8664615 - Flags: review+
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
Attachment #8665500 - Flags: review?(jonas)
Status: NEW → ASSIGNED
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.
Attachment #8665500 - Flags: review?(jonas) → review?(bkelly)
(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?
Flags: needinfo?(ehsan)
(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...
Flags: needinfo?(ehsan) → needinfo?(james)
(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
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).
Flags: needinfo?(james)
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!
Attachment #8665500 - Attachment is obsolete: true
Attachment #8665500 - Flags: review?(bkelly)
(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?
Flags: needinfo?(james)
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.
Flags: needinfo?(james)
https://hg.mozilla.org/mozilla-central/rev/bc7aa1e7670f
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.