1279208 - Favicon request doesn't timeout, or close when related window is closed (1255270 is not fixed yet)

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[:Gijs (out for now; he/him)]

+++ This bug was initially created as a clone of Bug #1255270 +++

In bug 1255270 comment 54 through to bug 1255270 comment 58, it's become clear the fix is not working correctly. We should address this.

Comment 1

[:Gijs (out for now; he/him)]

Attachment: Patch

Comment 2

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Al, Dan, is this something we should consider taking as a 47 dot release ride-along? It was nom'd for 47 tracking.

Comment 3

[Al Billings [:abillings - ex-MoCo]]

I would very much like this as part of 47 as I had to yank the advisory for the original report when it was clear it wasn't fixed.

Comment 4

[:Gijs (out for now; he/him)]

Comment on attachment 8761551 [details] [diff] [review]
Patch

Drew, do you feel comfortable and do you have cycles to review this "soon"? I think Marco's off to London already.

Comment 5

[:Gijs (out for now; he/him)]

Attachment: Patch

The more I think about this, the more I think we should always update the request for the favicon helper, or never update it, and this seems safest for now.

Comment 6

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Al Billings [:abillings] from comment #3)
> I would very much like this as part of 47 as I had to yank the advisory for
> the original report when it was clear it wasn't fixed.

Ok. Thanks! Let me include it in my list of 47 ride-alongs.

Comment 7

[Drew Willcoxon :adw]

Comment on attachment 8761787 [details] [diff] [review]
Patch

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

I can give an r+ for this or a version of it if you really need it, so let me know, but (1) I'm doubtful about two out of three of these changes, and (2) I don't know this code well so I think Marco should look at it.  I do think removing the win.top check is probably good.

::: browser/components/places/PlacesUIUtils.jsm
@@ +191,5 @@
>        gFaviconLoadDataMap.set(win, []);
>        let unloadHandler = event => {
>          let doc = event.target;
> +        let eventWin = doc.defaultView;
> +        if (eventWin == win && doc.documentURI != "about:blank") {

It looks like the about:blank term was already here but I don't see why it's necessary.  If anything maybe it hurts since maybe loadFavicon was called for an about:blank window.  The condition for whether the window is removed from gFaviconLoadDataMap when it's unloaded should only be whether it's present in gFaviconLoadDataMap.  Seems like.  (Which is why removing the win.top check seems like a good idea to me.)

@@ +208,5 @@
>      let callback = this._makeCompletionCallback(win, innerWindowID);
>      let request = PlacesUtils.favicons.setAndFetchFaviconForPage(currentURI, uri, false,
>                                                                   loadType, callback, principal);
> +    // Immediately cancel any earlier requests
> +    this.removeRequestsForInner(innerWindowID);

Seems reasonable I guess, but I wonder if this will actually make a difference.  If there are earlier requests, then they'll either finish before the window is destroyed, or if they don't, then they'll be canceled when the window is destroyed/unloaded.  If the latter doesn't happen when it should (which seems to be the point of this bug), then this change alone won't fix that for cases where loadFavicon isn't called again for the window.

::: toolkit/components/places/FaviconHelpers.cpp
@@ +442,5 @@
>    }
>  
> +  rv = channel->AsyncOpen2(this);
> +  if (NS_SUCCEEDED(rv)) {
> +    mRequest = channel;

This doesn't look right to me in a couple of ways.  First, it doesn't cancel the current mRequest if it's non-null.  Maybe that's not actually a problem because of the way this class is used, I don't know.  Second, it shouldn't be necessary at all because mRequest is set on OnStartRequest.  Is OnStartRequest called immediately after AsyncOpen2()?  I would bet not but I don't know, but even if not, Cancel properly handles the case where the instance is canceled before OnStartRequest is called by setting mCanceled to true, which is then handled in OnStartRequest.

So this doesn't look like it helps anything and it may actually hurt.

Comment 8

[:Gijs (out for now; he/him)]

(In reply to Drew Willcoxon :adw from comment #7)
> ::: toolkit/components/places/FaviconHelpers.cpp
> @@ +442,5 @@
> >    }
> >  
> > +  rv = channel->AsyncOpen2(this);
> > +  if (NS_SUCCEEDED(rv)) {
> > +    mRequest = channel;
> 
> This doesn't look right to me in a couple of ways.  First, it doesn't cancel
> the current mRequest if it's non-null.  Maybe that's not actually a problem
> because of the way this class is used, I don't know.

Right, I don't believe it is a problem.

>  Second, it shouldn't
> be necessary at all because mRequest is set on OnStartRequest.  Is
> OnStartRequest called immediately after AsyncOpen2()?  I would bet not but I
> don't know, but even if not, Cancel properly handles the case where the
> instance is canceled before OnStartRequest is called by setting mCanceled to
> true, which is then handled in OnStartRequest.

Sorry, the explanations for this were in the other bug. :-(

Effectively, the problem is that if you fire off 1000 of these, and then cancel them, which is what the testcase there does, then they end up being queued. OnStartRequest isn't fired until we actually start talking to a server (the name is misleading in the sense that it'll actually already have a response at that stage: https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Necko/Necko_walkthrough ). Which means you still end up connecting to the server 1000 times, even after the page is closed, because although we called Cancel(), that does nothing until OnStartRequest is fired. OnStartRequest is only fired when the request actually has HTTP headers, so it's actually reasonably late, and well after the request has hit the server on the other end of the connection. We don't want that - we shouldn't be contacting the server at all in this case.

If we set mRequest earlier, then the call to Cancel() on the helper object will immediately call Cancel() on the request, which leads to it being cancelled before it hits the server, which is the behaviour we're looking for.

Comment 9

[:Gijs (out for now; he/him)]

Attachment: Patch v1.1

(In reply to Drew Willcoxon :adw from comment #7)
> Comment on attachment 8761787 [details] [diff] [review]
> Patch
> 
> Review of attachment 8761787 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I can give an r+ for this or a version of it if you really need it, so let
> me know, but (1) I'm doubtful about two out of three of these changes, and
> (2) I don't know this code well so I think Marco should look at it.  I do
> think removing the win.top check is probably good.
> 
> ::: browser/components/places/PlacesUIUtils.jsm
> @@ +191,5 @@
> >        gFaviconLoadDataMap.set(win, []);
> >        let unloadHandler = event => {
> >          let doc = event.target;
> > +        let eventWin = doc.defaultView;
> > +        if (eventWin == win && doc.documentURI != "about:blank") {
> 
> It looks like the about:blank term was already here but I don't see why it's
> necessary.  If anything maybe it hurts since maybe loadFavicon was called
> for an about:blank window.  The condition for whether the window is removed
> from gFaviconLoadDataMap when it's unloaded should only be whether it's
> present in gFaviconLoadDataMap.  Seems like.  (Which is why removing the
> win.top check seems like a good idea to me.)

So the windows in the map are chrome windows. The point of the about:blank check was to avoid removing the window when an initial about:blank document unloads in favour of e.g. browser.xul, but I don't think that can actually happen, so I've updated the patch with this change.

As for timing... I don't know. I think at this stage, as it's only just morning here, the west coast is asleep, and I'm away this afternoon/evening, with London on our doorstep for next week, trying to get this ready before Monday is likely pointless anyway. Let's wait for Marco's review.

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 8761939 [details] [diff] [review]
Patch v1.1

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

::: browser/components/places/PlacesUIUtils.jsm
@@ +208,5 @@
>      let callback = this._makeCompletionCallback(win, innerWindowID);
>      let request = PlacesUtils.favicons.setAndFetchFaviconForPage(currentURI, uri, false,
>                                                                   loadType, callback, principal);
> +    // Immediately cancel any earlier requests
> +    this.removeRequestsForInner(innerWindowID);

should we do this before the favicon API call, just in case it throws (it shouldn't but...)?

::: toolkit/components/places/FaviconHelpers.cpp
@@ +442,5 @@
>    }
>  
> +  rv = channel->AsyncOpen2(this);
> +  if (NS_SUCCEEDED(rv)) {
> +    mRequest = channel;

is it still worth setting it also in onStartRequest? I'd rather change that assignment to a MOZ_ASSERT(mRequest);

Comment 11

[:Gijs (out for now; he/him)]

(In reply to Marco Bonardo [::mak] - Away 9-13 June from comment #10)
> Comment on attachment 8761939 [details] [diff] [review]
> Patch v1.1
> 
> Review of attachment 8761939 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/places/PlacesUIUtils.jsm
> @@ +208,5 @@
> >      let callback = this._makeCompletionCallback(win, innerWindowID);
> >      let request = PlacesUtils.favicons.setAndFetchFaviconForPage(currentURI, uri, false,
> >                                                                   loadType, callback, principal);
> > +    // Immediately cancel any earlier requests
> > +    this.removeRequestsForInner(innerWindowID);
> 
> should we do this before the favicon API call, just in case it throws (it
> shouldn't but...)?

Sure, we can do that.

> ::: toolkit/components/places/FaviconHelpers.cpp
> @@ +442,5 @@
> >    }
> >  
> > +  rv = channel->AsyncOpen2(this);
> > +  if (NS_SUCCEEDED(rv)) {
> > +    mRequest = channel;
> 
> is it still worth setting it also in onStartRequest? I'd rather change that
> assignment to a MOZ_ASSERT(mRequest);

I realized after the discussion earlier in the bug that I *think* we will normally get a second onStartRequest in the case of a redirect, with a new request that replaces the old request. In which case, we want to overwrite the request so that if we end up cancelling while the new request is pending, we call cancel() on the new rather than the old request. Does that sound right?

Comment 12

[:Gijs (out for now; he/him)]

Comment on attachment 8761939 [details] [diff] [review]
Patch v1.1

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: security issue
[Describe test coverage new/current, TreeHerder]: no. :-(
[Risks and why]: low risk, minor changes to existing code to ensure favicon requests don't outlive their tabs
[String/UUID change made/needed]: no

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
It should be fairly obvious what this is related to - but then, that was already the case for the original patch

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No tests, no comments, no check-in comment beyond the bug number and reviewer.

Which older supported branches are affected by this flaw?
Everything. :-(

If not all supported branches, which bug introduced the flaw?
n/a

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
They should be pretty easy. There's a pending esr backport in bug 1255270 which this should apply on top of as well.

How likely is this patch to cause regressions; how much testing does it need?
It should really get some testing. Unfortunately that didn't happen before releasing 1255270. We should do better this time.

Comment 13

[:Gijs (out for now; he/him)]

Talked with Marco out-of-band, need to add a comment but keep the re-assignment to mRequest.

Comment 14

[:Gijs (out for now; he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/6fb65c82c879

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6fb65c82c879
https://hg.mozilla.org/mozilla-central/rev/2dbae52b1ab8

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4ff1d42b56b9
https://hg.mozilla.org/releases/mozilla-aurora/rev/41ac93b90438

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ef99d8fb971d
https://hg.mozilla.org/releases/mozilla-beta/rev/b30c1d73a22a

Comment 18

[Georg Koppen]

Could we get something for ESR45 as well?

Comment 19

[:Gijs (out for now; he/him)]

(In reply to Georg Koppen from comment #18)
> Could we get something for ESR45 as well?

Well, the original patch hasn't landed there yet, and we're not landing this on 47... I'd like confirmation this is now actually fixed before trying to get this onto ESR. I'll look into it "tomorrow" (Tuesday).

Comment 20

[:Gijs (out for now; he/him)]

Attachment: Test zip file adapted from the other bug

So I changed the testcase a bit. It now fires one favicon request every 100ms off a setTimeout, to 'defeat' part of the fix I landed here (which immediately cancels all but the last request if you make them synchronously one after the other). I verified that as soon as the page has redirected or you close the tab, the requests stop. I also patched the python redir.py file so that it doesn't break quite as badly when the client (ie Firefox) closes the connection, though it still seems to not like being ctrl-c'd.

In any case, I compared beta before this fix (broken) and nightly after this fix (seems to disconnect fine), so I think this works. Given my earlier mess-ups in the previous bugs, I'd like confirmation from QA (Matt, ni'd you but please delegate if necessary) as well as Toni, if possible, that this is now OK. Toni, builds from https://nightly.mozilla.org should have this fix as of the 17th/18th should have this (so certainly an up-to-date, ie today's/yesterday's nightly should be fine to test with).

I'll prepare a patch for esr45 on the assumption that that testing succeeds, and I'll request approval if/when it does.

Comment 21

[:Gijs (out for now; he/him)]

Attachment: Patch for ESR 45

Comment 22

[Matt Wobensmith [:mwobensmith][:matt:]]

With the test files from comment 20 - thanks Gijs - I was able to observe the lack of disconnect after closing page, using the current release of Fx47. 

I confirmed the fix on today's Nightly 50.0a1.

Comment 23

[:Gijs (out for now; he/him)]

Comment on attachment 8763931 [details] [diff] [review]
Patch for ESR 45

If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high
User impact if declined: ditto
Fix Landed on Version: 50, uplifted to 48 and 49

Risk to taking this patch (and alternatives if risky): relatively large patch, certainly for ESR. Parts of it landed on 47 and were released there already. There is also some automated test coverage, so it's unlikely to break catastrophically, but there we are.

String or UUID changes made by this patch: Changes interface. No UUID change in the patch because we stopped doing that on m-c at the time of 47. Unclear whether you'd want me to change the UUID or rewrite the patch to not include a breaking interface change (ie adding a second method rather than modifying an existing one; this would be an extensive change and would weaken the impact of the patch to the point where e.g. add-ons might compromise its security benefit, and it would break compatibility of add-ons that rely on the new return value of the method we're changing on 49, when installed on 45).

Note also that while we changed an interface in this patchset, the change is only breaking for CPP consumers. JS consumers should not be affected unless they depended on the (formerly nonexistent) return value being, well, nonexistent, which seems extremely unlikely. There are no comm-central CPP consumers other than the ones from mozilla-central, which the patch fixes. Binary add-ons were deprecated well before 45. IMO, considering all of the above, rewriting the patch is not a good idea. I'm open to rev'ing the UUID, but I haven't done that in this patch - it would be a trivial change, though. Please let me know what you want.

(no string changes)

Comment 24

[Toni Huttunen]

I can't confirm the fix on today's Nightly 50.0a1. I still get requests 5min+ after page close. I tested using original tool and made same steps than before.

Comment 25

[:Gijs (out for now; he/him)]

Attachment: Screencast

(In reply to Toni Huttunen from comment #24)
> I can't confirm the fix on today's Nightly 50.0a1. I still get requests
> 5min+ after page close. I tested using original tool and made same steps
> than before.

This doesn't really make sense to me... I've attached a screencast of what I'm doing. The only change I made to the python script and html (freshly downloaded off bug 1255267) I'm using in the screencast is to replace example.com with 'localhost' in favicon.html so it actually contacts the python server, with no changes to favicon.html. The python output does not change after the end of the screencast for another 3-5 minutes, at which point I gave up. What am I doing wrong?

Comment 26

[Toni Huttunen]

Attachment: redir-log.txt

output of redir.py test case

Comment 27

[Toni Huttunen]

(In reply to :Gijs Kruitbosch from comment #25)
> Created attachment 8766018 [details]
> Screencast
> 
> (In reply to Toni Huttunen from comment #24)
> > I can't confirm the fix on today's Nightly 50.0a1. I still get requests
> > 5min+ after page close. I tested using original tool and made same steps
> > than before.
> 
> This doesn't really make sense to me... I've attached a screencast of what
> I'm doing. The only change I made to the python script and html (freshly
> downloaded off bug 1255267) I'm using in the screencast is to replace
> example.com with 'localhost' in favicon.html so it actually contacts the
> python server, with no changes to favicon.html. The python output does not
> change after the end of the screencast for another 3-5 minutes, at which
> point I gave up. What am I doing wrong?

Looks like in your screencast the html file is not spawning hundreds of requests to the redir.py when you launch it. I see there are only a couple of "connected" requests, when I do have hundreds. Could that be a platform specific issue? I'm testing on Win7.

Comment 28

[:Gijs (out for now; he/him)]

(In reply to Toni Huttunen from comment #27)
> (In reply to :Gijs Kruitbosch from comment #25)
> > Created attachment 8766018 [details]
> > Screencast
> > 
> > (In reply to Toni Huttunen from comment #24)
> > > I can't confirm the fix on today's Nightly 50.0a1. I still get requests
> > > 5min+ after page close. I tested using original tool and made same steps
> > > than before.
> > 
> > This doesn't really make sense to me... I've attached a screencast of what
> > I'm doing. The only change I made to the python script and html (freshly
> > downloaded off bug 1255267) I'm using in the screencast is to replace
> > example.com with 'localhost' in favicon.html so it actually contacts the
> > python server, with no changes to favicon.html. The python output does not
> > change after the end of the screencast for another 3-5 minutes, at which
> > point I gave up. What am I doing wrong?
> 
> Looks like in your screencast the html file is not spawning hundreds of
> requests to the redir.py when you launch it. I see there are only a couple
> of "connected" requests, when I do have hundreds. Could that be a platform
> specific issue? I'm testing on Win7.

I was really skeptical about this because the code I was touching should not be platform-specific, but for the second time you're absolutely correct and I need to eat some more humble pie (though in my defense, my thinking that the code I touched wasn't platform specific was also right...).

The reason there is *still* (ugh) an issue here is because beyond the actual favicon we show in the tab itself (which never caused a problem like this), and the favicon we fetch for history (which at least AFAICT is now properly fixed, but I spent a good hour trying to debug if anything was going wrong there), there's a third place where we request favicons - but it only exists on Windows. It's the windows per-tab taskbar preview ("Aero Peek") functionality. Which ironically is turned off by default because it has issues, but apparently still totally does network requests for this stuff (even when disabled), which is cringe-worthy. Code is in https://dxr.mozilla.org/mozilla-central/source/browser/modules/WindowsPreviewPerTab.jsm .

I'll file a third followup bug. :-\

Comment 29

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8763931 [details] [diff] [review]
Patch for ESR 45

Sec-high issues meet the ESR45 uplift bar. Assuming the sec-approval+ is carried over from the Beta/Aurora patch.

Comment 30

[:Gijs (out for now; he/him)]

Dan/Ritu: note that there's a remaining Windows-only issue (bug 1283067). Do you want this to land on esr now or wait for the other patch? I'm not sure how much time we have. :-\

Comment 31

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to :Gijs Kruitbosch from comment #30)
> Dan/Ritu: note that there's a remaining Windows-only issue (bug 1283067). Do
> you want this to land on esr now or wait for the other patch? I'm not sure
> how much time we have. :-\

Hi Gijs, we are still 3 weeks away from entering RC mode. I think we are ok to take uplifts until 7/25 (pending risk assessment). Please nominate a patch for uplift to ESR45 on bug 1283067 when ready. Thanks!

Comment 32

[Carsten Book [:Tomcat]]

i got 

remote: *** IDL file toolkit/components/places/mozIAsyncFavicons.idl altered in changeset 836e12576fa7IDL file toolkit/components/places/mozIPlacesPendingOperation.idl altered in changeset 836e12576fa7***
remote: 
remote: Changes to IDL files in this repo require you to provide binary change approval in your top comment in the form of ba=... (or, more accurately, ba\S*=...)
remote: This is to ensure that UUID changes (or method changes missing corresponding UUID change) are caught early, before release.


so has this binary approval ?

Comment 33

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Redirecting the NI to Jorge who can help us determine if the UUID/IDL change is going to affect add-on compatibility. Hi Jorge, is the ESR45 patch (especially UUID changes) OK to be uplifted to ESR45 branch?

Comment 34

[:Gijs (out for now; he/him)]

(In reply to Ritu Kothari (:ritu) from comment #33)
> Redirecting the NI to Jorge who can help us determine if the UUID/IDL change
> is going to affect add-on compatibility. Hi Jorge, is the ESR45 patch
> (especially UUID changes) OK to be uplifted to ESR45 branch?

I described add-on compat in comment #23. No JS-based add-ons would be affected unless they relied on the API always returning undefined, which seems unlikely.

Comment 35

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to :Gijs Kruitbosch from comment #34)
> (In reply to Ritu Kothari (:ritu) from comment #33)
> > Redirecting the NI to Jorge who can help us determine if the UUID/IDL change
> > is going to affect add-on compatibility. Hi Jorge, is the ESR45 patch
> > (especially UUID changes) OK to be uplifted to ESR45 branch?
> 
> I described add-on compat in comment #23. No JS-based add-ons would be
> affected unless they relied on the API always returning undefined, which
> seems unlikely.

My bad. Sorry I missed that. In that case, we should uplift it to ESR45. Tomcat, please go ahead and land this. Thanks!

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-esr45/rev/89dd6637b3a6

Comment 37

[Paul Silaghi, QA [:pauly]]

Requests are still made after the page redirection on FX 48b7 Win 7 x64.
Seems to work fine though on 49.0a2 (2016-07-14), 50.0a1 (2016-07-14) Win 7.
Thoughts?

Comment 38

[:Gijs (out for now; he/him)]

(In reply to Paul Silaghi, QA [:pauly] from comment #37)
> Requests are still made after the page redirection on FX 48b7 Win 7 x64.
> Seems to work fine though on 49.0a2 (2016-07-14), 50.0a1 (2016-07-14) Win 7.
> Thoughts?

(In reply to Carsten Book [:Tomcat] from comment #17)
> https://hg.mozilla.org/releases/mozilla-beta/rev/ef99d8fb971d

If I compare this to the aurora landing:

https://hg.mozilla.org/releases/mozilla-aurora/rev/4ff1d42b56b9

I think this wasn't rebased correctly for beta. I'm building beta locally right now, I'll land an update if the issue is trivial to fix (which is what I suspect).

Can you check esr45 as well?

Comment 39

[:Gijs (out for now; he/him)]

https://hg.mozilla.org/releases/mozilla-beta/rev/b9a4d34cb073

This should be fixed in beta 9 (release next Tuesday pacific time, AIUI). Leaving ni to verify that that is indeed the case (I verified myself locally, but it should be properly verified by QA) and that esr45 is already fixed (that was a separate patch so right now I expect no issues).

Comment 40

[Paul Silaghi, QA [:pauly]]

(In reply to :Gijs Kruitbosch from comment #38)
> (In reply to Paul Silaghi, QA [:pauly] from comment #37)
> > Requests are still made after the page redirection on FX 48b7 Win 7 x64.
> Can you check esr45 as well?
Same bad results on latest tinderbox esr45: https://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-esr45-win32/1468516304/.
You'll probably want to fix this too.

Comment 41

[:Gijs (out for now; he/him)]

(In reply to Paul Silaghi, QA [:pauly] from comment #40)
> (In reply to :Gijs Kruitbosch from comment #38)
> > (In reply to Paul Silaghi, QA [:pauly] from comment #37)
> > > Requests are still made after the page redirection on FX 48b7 Win 7 x64.
> > Can you check esr45 as well?
> Same bad results on latest tinderbox esr45:
> https://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-esr45-win32/
> 1468516304/.
> You'll probably want to fix this too.

Ugh, yeah, the ESR patch I attached is broken. I'll look into whether HG is buggy or if Tomcat and I made identical mistakes dealing with this patchset, or something -- after I fix up ESR.

Can you re-check beta against the tinderbox build so we're sure that my follow-up really really really fixed this there?

Comment 42

[Paul Silaghi, QA [:pauly]]

Confirmed it's fixed on latest beta tinderbox build https://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-beta-win32/1468744314/

Comment 43

[:Gijs (out for now; he/him)]

(In reply to Paul Silaghi, QA [:pauly] from comment #42)
> Confirmed it's fixed on latest beta tinderbox build
> https://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-beta-win32/
> 1468744314/

Excellent, thanks.

remote:   https://hg.mozilla.org/releases/mozilla-esr45/rev/4bd5a188c423

Re-ni for checking that after this, esr is fixed too (but this is basically the exact same changes that I pushed to beta, so I have high hopes...).

Comment 44

[Paul Silaghi, QA [:pauly]]

Verified fixed FX 45.2.1 ESR tinderbox-build from 18-Jul-2016.