1307122 - barmer.de: Clicking login button causes navigation to stop working

Status: VERIFIED FIXED

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

Description

[Tobias (:Tobbi) Markus]

Trying to login to barmer-gek.de currently hangs the content process.

Steps to reproduce:
1. Go to https://www.barmer-gek.de/ and click the Login button at the top right.
2. Notice that this hangs the content process and you cannot navigate away from this tab / click any links etc..

Comment 1

[Tooru Fujisawa [:arai]]

Here's regression range:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a52bf59965a0&tochange=084441e904d1

Comment 2

[Tooru Fujisawa [:arai]]

The issue doesn't happen if I disable JavaScript via Developer Tools options.

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: minimal testcase

the page does sync XMLHttpRequest to maybe-nonexistent host in beforeunload and unload event handlers.
so the handler doesn't return forever.

looks like almost a dupe of bug 705617,
but the regression range doesn't match.

Comment 4

[Tooru Fujisawa [:arai]]

in the testcase, I used https://webenfe.barmer-gek.de/ that is copied from the comment #0's website's case, the web server doesn't respond.
the testcase might become invalid if the server become online.

Comment 5

[Tobias (:Tobbi) Markus]

The testcase has gone invalid already. Can anyone find a replacement site?

Comment 6

[Andrew Overholt [:overholt]]

baku, could you please take a quick look here?

Comment 7

[Andrea Marchesini [:baku]]

I cannot reproduce the issue... Tobias, can you check archive.org?
NI me if we are able to reproduce the issue.

Comment 8

[Tooru Fujisawa [:arai]]

the issue is caused by the server down or something.
so archive.org won't be a solution.

we need either
  * webserver that doesn't respond (and firefox doesn't close connection)
  * webpage that accepts connection but doesn't return body forever

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: standalone testcase with server on node.js

here's standalone testcase, that uses webserver running on node.js.
the webserver accepts the connection, but doesn't respond.

test.html in it opens XHR to the webserver in beforeunload and unload handler.

Comment 10

[Andrea Marchesini [:baku]]

Our implementation follows the spec. I don't see why we should close the connection if a sync XHR is used in unload or in beforeunload.

Comment 11

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

(sync XHR during unload were the reason to add sendBeacon).

What do other browsers do here?

We could add some ugly timeout hack here to cancel the XHR eventually. If the testcase is really what the website is doing, it is behaving badly, but perhaps we should have some mitigation.
We could possible use the timeout property of XHR. It is only for async XHR when JS uses it, but perhaps internally we could set it when document is being unloaded or beforeunloaded.

Comment 12

[Andrea Marchesini [:baku]]

This seems a reasonable approach.
annevk, do you have any suggestion here?

Comment 13

[Anne (:annevk)]

Not really. Also somewhat curious what other browsers do.

Comment 14

[Tobias (:Tobbi) Markus]

Chromium's back button navigation also appears to not be working. However, at least address bar navigation works.

I filed https://bugs.chromium.org/p/chromium/issues/detail?id=652318 on Chromium's bugtracker.

Comment 15

[Andrea Marchesini [:baku]]

I would like to go for what smaug suggested in comment 11.

Comment 16

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

I wonder if Chrome can load the next page only if it ends up using a new process.
In cases it can't use (say, you have been opened by another tab so you have .opener and you're trying to load a same origin page), does it perhaps break the same way as FF.

Comment 17

[Andrea Marchesini [:baku]]

Attachment: timeout.patch

This fixes the testcase but it doesn't fix a simple:

for (;;) {
  var xhr = new XMLHttpRequest();
  xhr.open("GET", something, false);
  try { xhr.send(); } catch(e) {}
}

Comment 18

[Andrea Marchesini [:baku]]

Actually, that script is blocked in non-e10s. I'm debugging how we manage the slow scripts in e10s mode.

Comment 19

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

Comment on attachment 8801034 [details] [diff] [review]
timeout.patch

>+    mUnloadEventsTimeStamp(0)
Could you call the variable something like
mBeforeUnloadOrUnloadEventTimeStamp
(but see a comment later)

>+  // This helper class must be set when we dispatch beforeunload and unload
>+  // events in order to avoid unterminate sync XHRs.
>+  class MOZ_RAII WhenUnloadEventsHelper
odd name, IMO. Perhaps BeforeUnloadOrUnloadEventTimeStamp or so something like that
(but see a comment later)

>+    {
>+      MOZ_ASSERT(aDocument);
>+      mDocument->SetUnloadEventsTimeStamp(PR_Now());
This kind of stuff really should use TimeStamp, and NowLoRes() would be fine for this.

>+  PRTime UnloadEventsTimeStamp() const
So this should return TimeStamp()


>-    {
>+    SyncTimeoutType syncTimeoutType = MaybeStartSyncTimeoutTimer();
>+    if (syncTimeoutType == eErrorOrExpired) {
>+      CloseRequestWithError(ProgressEventType::abort);
I think you should just explicitly call Abort()

>   //   these items are specific to markup documents (html and xml)
>   //   may consider splitting these out into a subclass
>   unsigned      mIsSticky : 1;
>-  unsigned      mInPermitUnload : 1;
What has this change to do with this patch?

> nsDocumentViewer::GetBeforeUnloadFiring(bool* aInEvent)
> {
>-  *aInEvent = mInPermitUnload;
>+  *aInEvent = mDocument && mDocument->UnloadEventsTimeStamp();
This is wrong since UnloadEventsTimeStamp is set also when doing unload, not only beforeunload.

>@@ -1313,16 +1312,18 @@ nsDocumentViewer::PageHide(bool aIsUnloa
Ahaa, this method name reminds... we should handle also pagehide event.
That is dispatched in nsDocument.cpp

So, need to think about naming a bit.
Perhaps WhenUnloadEventsHelper should become PageUnloadingEventTimeStamp or some such.
And similar naming elsewhere.

Comment 20

[Andrea Marchesini [:baku]]

Attachment: timeout.patch

Comment 21

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

Comment on attachment 8801116 [details] [diff] [review]
timeout.patch

based on irc, I'm expecting to see yet another patch.

Comment 22

[Andrea Marchesini [:baku]]

Attachment: timeout.patch

Comment 23

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

Comment on attachment 8801173 [details] [diff] [review]
timeout.patch

>-  DispatchPageTransition(target, NS_LITERAL_STRING("pagehide"), aPersisted);
>+  {
>+    PageUnloadingEventTimeStamp(this);
This shouldn't be a temporary variable, but you need PageUnloadingEventTimeStamp timeStamp(this);
or so.

>+  mozilla::TimeStamp UnloadEventsTimeStamp() const
Call this GetPageUnloadingEventTimeStamp
or something to be more consistent with naming

>+  void SetUnloadEventsTimeStamp()
SetPageUnloadingEventTimeStamp



>+  mozilla::TimeStamp mBeforeUnloadOrUnloadEventTimeStamp;
mPageUnloadingEventTimeStamp

> #define NS_PROGRESS_EVENT_INTERVAL 50
>+#define MAX_SYNC_TIMEOUT_WHEN_UNLOADING 5000
Feels still a bit low on slow connections. 10000 perhaps? This is after all something which well behaving websites shouldn't trigger.

>+    SyncTimeoutType syncTimeoutType = MaybeStartSyncTimeoutTimer();
>+    if (syncTimeoutType == eErrorOrExpired) {
>+      CloseRequestWithError(ProgressEventType::abort);
Why not Abort()?

>+XMLHttpRequestMainThread::HandleSyncTimeoutTimer()
>+{
>+  MOZ_ASSERT(mSyncTimeoutTimer);
>+  MOZ_ASSERT(mFlagSyncLooping);
>+
>+  CancelSyncTimeoutTimer();
>+  CloseRequestWithError(ProgressEventType::abort);
Why not Abort() ?

those fixed, r+

Comment 24

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

Could we get some test for this? (I would expect .sjs could be used for the test)

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/94f47e107744
Introducing a timeout for sync XHR when unload events are dispatched, r=smaug

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/94f47e107744

Comment 27

[Gerry Chang [:gchang]]

Hi :Tobbi,
could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 28

[Tobias (:Tobbi) Markus]

(In reply to Gerry Chang [:gchang] from comment #27)
> Hi :Tobbi,
> could you please verify this issue is fixed as expected on a latest Nightly
> build? Thanks!

I was able to verify the fix with the attached testcase. Thanks!

Comment 29

[Gerry Chang [:gchang]]

Hi :baku,
Since this bug is a regression, do you consider to uplift this for 51 aurora at least?

Comment 30

[Andrea Marchesini [:baku]]

I don't think this is a regression, actually, but still, we could uplift this for aurora.

Comment 31

[Andrea Marchesini [:baku]]

Comment on attachment 8801173 [details] [diff] [review]
timeout.patch

Approval Request Comment
[Feature/regressing bug #]: Sync XHR
[User impact if declined]: The tab can freeze if a sync XHR is done in unload event and the server doesn't answer.
[Describe test coverage new/current, TreeHerder]:green on try. Test is part of the patch.
[Risks and why]: not too much. The patch introduces a concept of timer for doing sync XHRs when unloading.
[String/UUID change made/needed]: none

Comment 32

[Gerry Chang [:gchang]]

Comment on attachment 8801173 [details] [diff] [review]
timeout.patch

Fix an issue relate to sync XHR and was verified. Take it in 51 aurora.

Comment 33

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/d33b1329a44d

Comment 34

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

Too late to fix in 50.1.0 release