<a class="header-button" href="https://bugzilla.mozilla.org/home" title="Go to home page"> Bugzilla

Ryan VanderMeulen [:RyanVM]

Comment 3

•

6 years ago

FYI, This is still reproducible on Bughunter. I've resubmitted 75 urls where this has happened in the past on both Linux and Windows and was able to reproduce on 44 of them. http://russnov.ru/vecher-s-vladimirom-solovevym-03-07-2019/ is an example.

Updated

•

6 years ago

Priority: P5 → --

Comment 4

•

6 years ago

Bob, I tried that link from comment 3 in a debug browser built from revision a7ca5ad33f3d on Linux and I am not getting an assertion...

Any idea what I should do to be able to reproduce the assertion? Is it happening for you reliably on that site?

Flags: needinfo?(bob)

Comment 5

•

6 years ago

I was using a Fedora and a build from yesterday this morning when I picked that url and it was pretty reproducible just by loading the url and perhaps refreshing once or twice but I can't get it or the others top reproducible urls to fire the assert again. I've resubmitted the 75 urls again to see there is another that I can confirm still reproduces.

http://russnov.ru/vecher-s-vladimirom-solovevym-03-07-2019/
http://tadslgbtmusiccharts.blogspot.com/2019/09/chart-week-38-week-of-22nd-september.html#comment-form
http://tix-iyo-tiraab.blogspot.com/2018/01/ifb-compilation-197080s_15.html
http://evasbackparty.de/choco-fresh-torte/
https://achigawacell.blogspot.com/2018/03/cara-flash-samsung-galaxy-s4-gt-i9500.html

Comment 6

•

6 years ago

I've also gone back to the entire history and tested the full 140+ urls where this has occurred in the past. Bughunter can reliably reproduce it but I am unable to do so locally. The two most frequently reproduced urls are http://aops..uscg.mil/ and http://.google.hu/ which are invalid urls and only load when I use the bughunter proxy.

The next most frequently reproduced urls are

I wonder if the proxy isn't serving me stale content and the current versions of the sites no longer have whatever advertisement caused the assertion.

cknowles: Can we make the proxy always serve fresh content? What do you think is up with the malformed urls like http://aops..uscg.mil/ and http://.google.hu/ ? When I load them with an opt build, the proxy returns a 400 Bad Request error with X-Squid-Error
ERR_INVALID_URL 0

bz: Perhaps you can proxy through a squid instance somewhere and try one of the invalid urls http://aops..uscg.mil/ and http://.google.hu/ ?

Flags: needinfo?(bob) → needinfo?(cknowles)

Chris Knowles [:cknowles]

Comment 7

•

6 years ago

Sadly, we already do this:
cache deny all
Is on the config for the proxy.

Per https://wiki.squid-cache.org/SquidFaq/ConfiguringSquid#Can_I_make_Squid_proxy_only.2C_without_caching_anything.3F and other squid docs, that's the correct approach for the version we're on.

Looking at the logs, I don't see any cache hits.

I think the issue is that the squid box is trying to resolve the DNS name - and rightly failing due to the malformation. Of course, I can't even open those URLs on a test VM I have.

Let me know if I can look at anything else for you.

Flags: needinfo?(cknowles)

Comment 8

•

6 years ago

I'm not quite sure where to find a random squid instance...

Comment 9

•

6 years ago

I did a quick dnf install and just ran squid from the command line. Weirdly I get a different assertion failure than the one I got with the bughunter proxy when loading the invalid urls.

Assertion failure: cache->GetWrapperMaybeDead() == obj || (js::RuntimeIsBeingDestroyed() && !cache->GetWrapperMaybeDead()), at /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/BindingUtils.h:1378

I'll keep looking for reproducible urls and will report back if I find them.

Comment 12

•

6 years ago

bz: Just reproduced twice in a row on Ubuntu 19.10 Nightly with https://www.baiscopelk.com/
Get it while it's hot.

Assertion failure: XRE_IsParentProcess(), at /builds/worker/workspace/build/src/toolkit/components/windowwatcher/nsWindowWatcher.cpp:700
#01: nsWindowWatcher::OpenWindow(mozIDOMWindowProxy*, char const*, char const*, char const*, nsISupports*, mozIDOMWindowProxy**) (/mozilla/builds/nightly/mozilla/firefox-debug/dist/bin/libxul.so)
#02: NS_InvokeByIndex (/mozilla/builds/nightly/mozilla/firefox-debug/dist/bin/libxul.so)

Flags: needinfo?(bzbarsky)

Comment 13

•

6 years ago

I tried that URL a few times on Linux (under rr) now, starting 5 minutes after comment 12, and haven't been able to reproduce. :(

Flags: needinfo?(bzbarsky)

Comment 14

•

6 years ago

I can still reliably reproduce on the ubuntu 19.10 vm but I can run rr on the vm so I can't check it there. I can't reproduce on my local laptop running fedora 30 at all. What distro are you using?

Comment 15

•

6 years ago

cknowles got the vm configured temporarily to run rr and I was able to get the assertion. If there is a file or directory I can share with you or if you want to hit me on irc and drive me like the bot I am, let me know.

Comment 16

•

6 years ago

Thank you! I'm on Fedora 30 as well.

If you can rr pack the trace directory involved and then zip it up and get it to me, that would be really helpful. Also the version of rr that was used to record.

Comment 17

•

6 years ago

Ok, Bob and I stepped through this in rr and we think we know what's going on:

An iframe is created in a web page (from the parser). It starts a load of its src attribute.
That load comes back as a redirect and necko starts its various redirect processing. In particular, it calls HttpChannelChild::Redirect1Begin which calls HttpChannelChild::SetupRedirect which creates the new channel and calls SetupReplacementChannel to set up its loadgroup, etc. After this the redirection process goes async.
While that async work is going on, the iframe element is removed from the web page. That tears down the docshell, which calls Cancel on the docshell's loadgroup. This cancels the pre-redirect channel, but not the post-redirect one, since the latter is not in the loadgroup yet! Docshell teardown nulls out the mDocShell member of the nsDSURIContentListener.
The async redirect work finishes up and necko calls into HttpChannelChild::CompleteRedirectSetup which adds the post-redirect channel to the loadgroup. There are comments in there about how "No need to check for cancel", but those really don't seem right to me... At this point the pre-redirect channel has NS_BINDING_ABORTED as its status, as expected.
The post-redirect channel sends an OnStartRequest to its listener, and its status is NOT a failure status at this point, contrary to the comments in CompleteRedirectSetup. So the URILoader tries to handle the load, the nsDSURIContentListener doesn't want to handle it (because it has a null mDocShell, per item 3), and we end up trying to hand it off to the helper app service, triggering the assert.

I would have thought that HttpChannelChild::Cancel would call Cancel on mRedirectChannelChild if it's non-null... Or that something would prevent the redirect from completing successfully if Cancel is called on the pre-redirect channel before the post-redirect channel is added to the loadgroup, at least.

Honza, could you take a look? I think you know this stuff best... If it would be useful, Bob still has the live rr replay and you could poke around in there.

Component: Window Management → Networking: HTTP

Flags: needinfo?(honzab.moz)

Comment 18

•

6 years ago

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #17)

Ok, Bob and I stepped through this in rr and we think we know what's going on:

An iframe is created in a web page (from the parser). It starts a load of its src attribute.

That load comes back as a redirect and necko starts its various redirect processing. In particular, it calls HttpChannelChild::Redirect1Begin which calls HttpChannelChild::SetupRedirect which creates the new channel and calls SetupReplacementChannel to set up its loadgroup, etc. After this the redirection process goes async.

While that async work is going on, the iframe element is removed from the web page. That tears down the docshell, which calls Cancel on the docshell's loadgroup. This cancels the pre-redirect channel, but not the post-redirect one, since the latter is not in the loadgroup yet! Docshell teardown nulls out the mDocShell member of the nsDSURIContentListener.

The async redirect work finishes up and necko calls into HttpChannelChild::CompleteRedirectSetup

The cancellation of the pre-redirect channel on the child process happens between the async redirect decision for the post-redirect child channel has been made and Redirect3Complete message has been received back on the child (what happens after the post-redirect nsHttpChannel on the parent process has been succesfully asyncopened in reaction to accepting the redirect also on the child).

which adds the post-redirect channel to the loadgroup. There are comments in there about how "No need to check for cancel", but those really don't seem right to me... At this point the pre-redirect channel has NS_BINDING_ABORTED as its status, as expected.

You are right! Very good catch. The comment is here: https://searchfox.org/mozilla-central/rev/088e2cf29c59d733d57af43903eb0267dbf72e2a/netwerk/protocol/http/HttpChannelChild.cpp#2249-2254 and should be removed or altered to reflect the changes I suggest below.

The post-redirect channel sends an OnStartRequest to its listener, and its status is NOT a failure status at this point, contrary to the comments in CompleteRedirectSetup. So the URILoader tries to handle the load, the nsDSURIContentListener doesn't want to handle it (because it has a null mDocShell, per item 3), and we end up trying to hand it off to the helper app service, triggering the assert.

I would have thought that HttpChannelChild::Cancel would call Cancel on mRedirectChannelChild if it's non-null... Or that something would prevent the redirect from completing successfully if Cancel is called on the pre-redirect channel before the post-redirect channel is added to the loadgroup, at least.

Honza, could you take a look? I think you know this stuff best... If it would be useful, Bob still has the live rr replay and you could poke around in there.

I believe the right fix here is to simply throw the post-redirect child channel away (it's not in the load group, doesn't refer the listener and has never been asyncopened) when the pre-redirect child channel is found cancelled in HttpChannelChild::Redirect3Complete; more or less copy what happens here for succeeded == false: https://searchfox.org/mozilla-central/rev/088e2cf29c59d733d57af43903eb0267dbf72e2a/netwerk/protocol/http/HttpChannelParent.cpp#2145, never send the event.

I don't think this is a security issue.

Flags: needinfo?(honzab.moz)

Priority: -- → P2

Whiteboard: [necko-triaged]

Comment 19

•

6 years ago

Honza: Do you still need the rr recording?

Flags: needinfo?(honzab.moz)

BugBot [:suhaib / :marco/ :calixte]

Comment 20

•

6 years ago

•

Edited

(In reply to Bob Clary [:bc:] from comment #19)

Honza: Do you still need the rr recording?

I believe you can delete it. It's possible to write a test for the scenario, so we will be able to verify the cause and the fix.

And thanks!

Flags: needinfo?(honzab.moz)

Daniel Veditz [:dveditz]

Updated

•

6 years ago

Group: network-core-security

Keywords: testcase-wanted

Updated

•

6 years ago

Crash Signature: [@ nsWindowWatcher::OpenWindowInternal(mozIDOMWindowProxy *,char const *,char const *,char const *,bool,bool,bool,nsIArray *,bool,bool,bool,nsDocShellLoadState *,mozilla::dom::BrowsingContext * *)]

Comment 22

•

6 years ago

I don't think this is a security issue.

Are you sure? This is violating assertions in the WindowWatcher; do you understand what the implications of that are? I know I don't...

Flags: needinfo?(honzab.moz)

Comment 23

•

6 years ago

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #22)

I don't think this is a security issue.

Are you sure? This is violating assertions in the WindowWatcher; do you understand what the implications of that are? I know I don't...

Thanks for stepping in, Boris. I was mostly reading only the findings from your comment 17 and not much the rest of the bug. Thinking about it more, I am not able to assess if this is a security bug or not, to be honest. The assertion was added in bug 1261842, which was not a security bug and thus a comment for the assertion could have been added, but was not. Hard to say why this is expected only on the parent process and what the implications of violating it are.

This has been prioritized as a P2 in Necko. IMO, if there is a security bug then it's more likely in the response handling (doc nav) than in Necko itself. The necko bug mostly just uncovers that the paths above necko are vulnerable.

I think this should move back to window management component, and try to find a fix in ww, and we should also file a new bug for the necko issue to fix what you have found in comment 17.

What do you think?

Flags: needinfo?(honzab.moz) → needinfo?(bzbarsky)

Comment 24

•

6 years ago

IMO, if there is a security bug then it's more likely in the response handling

I mean, necko is violating its contract, which makes it really hard to reason about anything in the higher layers. There are lots of places where we assume that once we cancel a channel it is in fact canceled!

I think we just need to fix this on the necko side; it's really not clear to me what the higher layers could do here without breaking all sorts of other cases where we really do want to process a response from necko.

Flags: needinfo?(bzbarsky)

Comment 25

•

6 years ago

Got it. If this can only trigger because of necko contract violation and there is nothing else for the consumer but keeping some additional information about the channel cancellation (which is actually pretty redundant), then this is solely a necko bug, yes. Please feel free to nominate as a P1 so we can find an assignee and fix for this release.

Comment 26

•

6 years ago

Right, on the consumer side what this looks like is that we cancel a channel and then later we still get an OnStartRequest with a non-error status. How do I nominate as P1?

Comment 27

•

6 years ago

Ah, I mean to simply change the priority to P1. Done and ni? Nhi to find an assignee.

Flags: needinfo?(nhnguyen)

Priority: P2 → P1

Nhi Nguyen (:nhi)

Updated

•

6 years ago

Assignee: nobody → kershaw

Flags: needinfo?(nhnguyen)

Kershaw Chang [:kershaw]

Assignee

Comment 28

•

6 years ago

Attached file Bug 1587686 - Cancel post-redirect channel if pre-redirect channel is already canceled — Details

Kershaw Chang [:kershaw]

Assignee

Comment 29

•

6 years ago

(In reply to Honza Bambas (:mayhemer) from comment #20)

(In reply to Bob Clary [:bc:] from comment #19)

Honza: Do you still need the rr recording?

I believe you can delete it. It's possible to write a test for the scenario, so we will be able to verify the cause and the fix.

It looks like there is no easy way to write a test for this. To reproduce this, the pre-redirect channel has to be canceled after SendRedirect2Verify.

https://hg.mozilla.org/mozilla-central/rev/4e86786de847

Comment 30

•

6 years ago

(In reply to Kershaw Chang [:kershaw] from comment #29)

(In reply to Honza Bambas (:mayhemer) from comment #20)

(In reply to Bob Clary [:bc:] from comment #19)

Honza: Do you still need the rr recording?

I believe you can delete it. It's possible to write a test for the scenario, so we will be able to verify the cause and the fix.

It looks like there is no easy way to write a test for this. To reproduce this, the pre-redirect channel has to be canceled after SendRedirect2Verify.

OK, if this is too hard, just leave it. Although, I was thinking about having a custom redirect veto handler added that would sync somehow between the two processes. It's easy now to do messaging in xpcshell tests for e10s. But that was just a thought and maybe it's really not that easy.

Pulsebot

Comment 31

•

6 years ago

Pushed by kjang@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4e86786de847 Cancel post-redirect channel if pre-redirect channel is already canceled r=mayhemer

Andreea Pavel [:apavel]

Comment 32

•

6 years ago

bugherder

Status: NEW → RESOLVED

Closed: 6 years ago

status-firefox72: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla72

Ryan VanderMeulen [:RyanVM]

Updated

•

6 years ago

status-firefox70: --- → wontfix

status-firefox71: --- → wontfix

status-firefox-esr68: --- → wontfix

Keywords: assertion