1126851 - Using nightly 38.0a1 (2015-01-28) window.location.reload fails following an XHR post

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Stuart Colville [:muffinresearch]]

See bug 1125828 for the issue affecting marketplace.

For the login to FXA we do the login in a popup and then the window postMessages the main window at which point we finish the login and close the popup. The code that finishes the login does a POST via XHR. After the post window.location.reload() is called.

As of Nightly FF38 it seems that window.location.reload() is a no-op. It works run from the console out of the context of the scenario described above.

Running window.location.reload(true) made no difference but window.location=window.location did reload the page.

I'm not sure as to whether this expected or a regression in nightly. All older versions (<FF38) do the reload as expected. 

Please assign a different component if there's a better fit than general.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Stuart, is it possible to post or link to a testcase that shows this problem?  Or even steps to reproduce?  The ones in bug 1125828 seem to assume a lot of context I don't have.

From the description it's not clear whether you are doing the window.location.reload() after doing window.close().  Are you?

Comment 2

[Kevin Brosnan [Ex-Mozilla]]

Running mozRegression to find a range should get it down to a couple changesets assuming this is recent. http://mozilla.github.io/mozregression/install.html

Comment 3

[Stuart Colville [:muffinresearch]]

When I refer to changing window.location.reload() to other things I'm directly modifying this line: https://github.com/mozilla/zamboni/blob/master/media/js/devreg/login.js#L155 and replacing the prod JS with a locally built copy of the JS.


STR: 

 * Open https://marketplace-dev.allizom.org/developers/ in nightly
 * Click sign-in
 * Enter credentials in the FxA popup

What should happen:

 * The main page should reload so that the login state is reflected

What happens:

 * No reload of the page occurs.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Looking at login.js, it looks like the POST XHR is happening in the popup's parent, not in the popup, so the close() bit doesn't look immediately relevant, ok.

> * Enter credentials in the FxA popup

Do we have a test account that can be used, or does anyone testing this need to create an account on marketplace?

Tracy, Anthony, do you have time to look into this in terms of finding a regression range?

Comment 5

[Stuart Colville [:muffinresearch]]

(In reply to Boris Zbarsky [:bz] from comment #4)
> Looking at login.js, it looks like the POST XHR is happening in the popup's
> parent, not in the popup, so the close() bit doesn't look immediately
> relevant, ok.
> 
> > * Enter credentials in the FxA popup
> 
> Do we have a test account that can be used, or does anyone testing this need
> to create an account on marketplace?

Yep, you'll need to create a Firefox Account as a pre-requisite for testing this.

> Tracy, Anthony, do you have time to look into this in terms of finding a
> regression range?

Comment 6

[u279076]

(In reply to Boris Zbarsky [:bz] from comment #4)
> Tracy, Anthony, do you have time to look into this in terms of finding a
> regression range?

I can take a crack at this. I'll report back when I have a range.

Comment 7

[u279076]

I can't seem to reproduce this using the steps in comment 3. I tested Firefox 35.0.1 and today's 38.0a1, the experience was the same in both: the page is reloaded and my state shows signed in. Can I please get clearer steps to reproduce?

Comment 8

[Boris Zbarsky [:bzbarsky]]

> I'll report back when I have a range.

Thanks!  Please needinfo me then!

Comment 9

[u279076]

(In reply to Boris Zbarsky [:bz] from comment #8)
> > I'll report back when I have a range.
> 
> Thanks!  Please needinfo me then!

Hi Boris, I'm not sure if you missed it but I've hit a roadblock to investigating the regression window (see comment 7).

Comment 10

[Boris Zbarsky [:bzbarsky]]

I did see that, yes.  I'm assuming that Stuart will be able to provide steps to reproduce that reproduce the problem.

Comment 11

[Stuart Colville [:muffinresearch]]

Here's a video showing the bug: https://www.dropbox.com/s/fonrfu00wg1jagu/sign-in-button-new-prof.mp4?dl=0

This is on OSX and using a brand new profile to make sure no plugins are affecting the process.

One thing to double-check is the URL as the bug doesn't occur on https://marketplace-dev.allizom.org but *does* occur on https://marketplace-dev.allizom.org/developers/

Let me know if you need any more info.

Comment 12

[u279076]

(In reply to Stuart Colville [:scolville] [:muffinresearch] from comment #11)

Thanks for clarifying. I'm still not able to reproduce this on Linux but I am able to reproduce it on Mac OS X. I don't have a Windows machine in front of me right now so I cannot confirm if this is Mac-only, though it seems like Linux at least is not affected.

I will try to find a regression window on Mac.

Comment 13

[u279076]

Last good revision: 3c296aa11c51 [2015-01-01]
First bad revision: 13fe5ad0364d [2015-01-02]
Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3c296aa11c51&tochange=13fe5ad0364d

Nothing immediately stands out to me in the log, except for maybe bug 1102050. What do you think, Boris?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Hmm.  Yeah, nothing is jumping out at me in that range.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Alright, so...

First of all, I can confirm the regression range.

When we land in location.reload(), the window is handling a resize event, or so it thinks.  That is, its mIsHandlingResizeEvent is true.  This short-circuits the reload() call, per spec.  

The interesting thing is that I don't see the breakpoint in nsGlobalWindow::PreHandleEvent that sets mIsHandlingResizeEvent to true hit at any point after the popup opens.

Comment 16

[Boris Zbarsky [:bzbarsky]]

And for extra fun, in a debug build from changeset 13fe5ad0364d I am not seeing the problem so far...

Comment 17

[Boris Zbarsky [:bzbarsky]]

And likewise with a local opt build.  And likewise with a clean profile.

I _can_ reproduce in a debug build on tip, though.  Maybe I'll try bisecting when that started happening...

Comment 18

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]: Web compat regression.

Alright.  So a local bisect with a debug build leads me to:

The first bad revision is:
changeset:   225072:afa74310b970
user:        Andreas Pehrson <[e-mail]>
date:        Thu Dec 04 02:01:00 2014 +0100
summary:     Bug 992685 - Part 2. Fire resize event when video size changes. r=roc

which makes a lot more sense, in that at least it has to do with resize events.  This was checked in _much_ more recently than the regression range in comment 13, so I don't know what Anthony and I were seeing in those nightlies...

In any case, on tip here's what's going on:

During the pageload of https://marketplace-dev.allizom.org/developers/ a bunch of resize events are fired from HTMLMediaElement::MetadataLoaded (there are some videos on this page?).  These events trigger nsGlobalWindow::PreHandleEvent, since that's called all the way up the event target chain, and hence set mIsHandlingResizeEvent to true.  

But nsGlobalWindow::PostHandleEvent is never called, because that's only called up the target chain for bubbling events, and the events HTMLMediaElement::DispatchEvent fires are non-bubbling.

So we get stuck with mIsHandlingResizeEvent set to true.

Olli, can we just filter out resize events not targeted at the window in PreHandleEvent here?

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Yes, we should do that.
mIsHandlingResizeEvent should be set only if the event is trusted and its originalTarget is 
a window object.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Attachment: only handle Window level resize events in nsGlobalWindow

https://tbpl.mozilla.org/?tree=Try&rev=88ef96a56013

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8559252 [details] [diff] [review]
only handle Window level resize events in nsGlobalWindow

Why can't we just check |aVisitor.mEvent->originalTarget == this| instead of doing a QI and whatnot?

r=me, though it might be nice to add a test...

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

I just wanted to keep the old behavior where we set the flag if any of the child windows do resize.
Maybe I shouldn't worry about that.

Yeah, I'll add a test.


I didn't check if the patch helps with this bug, but the patch does fix obvious issue.

Comment 23

[Boris Zbarsky [:bzbarsky]]

> I just wanted to keep the old behavior where we set the flag if any of the child windows
> do resize.

We do?  I didn't think we bubbled that up to the parent window...

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

It propagates to the chrome window.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Ah, right, ok.

I guess that's not web-observable, so it doesn't matter too much in terms of web and spec compat what we do there.  And people shouldn't be reloading chrome windows anyway (yeah, I know).

Alright, let's keep the QI but add a comment explaining what's going on there?

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

Attachment: +test

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

https://tbpl.mozilla.org/?tree=Try&rev=281d174c9448

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

Attachment: +test

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c11024ebd04

Stuart, could you test tomorrow's nightly.

Comment 30

[Stuart Colville [:muffinresearch]]

Tried today with 38.0a1 (2015-02-06) and it's still not working but, judge by Comment 29, I think this was too soon. Will check on Monday.

Comment 31

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6c11024ebd04

Comment 32

[Stuart Colville [:muffinresearch]]

(In reply to Olli Pettay [:smaug] from comment #29)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/6c11024ebd04
> 
> Stuart, could you test tomorrow's nightly.

Using 38.0a1 (2015-02-08) this works great. Thanks!

Comment 33

[Liz Henry (:lizzard)]

Removing the tracking request since this was fixed while 38 was in Nightly.