Athena transport is not closed on page unload in IE

I can't read the change description in the ticket description. I've never seen a diff that looks like that. Can you provide any change description in the form of a unified or contextual diff?

Replying to exarkun:

I can't read the change description in the ticket description. I've never seen a diff that looks like that. Can you provide any change description in the form of a unified or contextual diff?

OK.

site-packages/nevow/js/Nevow/Athena > diff -C 4 __init__.js.original __init__.js
*** __init__.js.original        2008-09-19 18:32:57.000000000 +0200
--- __init__.js 2008-09-20 13:24:57.000000000 +0200
***************
*** 626,633 ****
--- 626,634 ----
       * final basket case is special: its sequence identifier is "close", as is
       * its message.
       */
      function sendCloseMessage(self) {
+     //alert('sendCloseMessage');
          self.stop();
          self.outputFactory(true).send(self.ack, [["unload", ["close", []]]]);
      });

***************
*** 1448,1455 ****
--- 1449,1458 ----

  Nevow.Athena.Widget._initialize = function() {
      Divmod.debug("widget", "Instantiating live widgets");
      Nevow.Athena.Widget._pageLoaded = true;
+     // With the next line, sendCloseMessage will get called in IE. // Harald
+     Divmod.Base.addUnLoadEvent(Nevow.Athena.page.deliveryChannel); // Harald
      Nevow.Athena.Widget._instantiateWidgets();
      Divmod.debug("widget", "Finished instantiating live widgets");
  };

***************
*** 1470,1477 ****
--- 1473,1481 ----
  Nevow.Athena.bootstrap = function (pageClassName, clientID) {
      var self = this;
      var pageClass = Divmod.namedAny(pageClassName);
      self.page = pageClass(clientID, Nevow.Athena._createMessageDelivery);
+     Nevow.Athena.page = self.page; // Harald
      self.page.bindEvents(window);

      /* Delay initialization for just a moment so that Safari stops whirling
       * its loading icon.



site-packages/nevow/js/Divmod > diff -C 4 Base.js.original Base.js
*** Base.js.original    2008-09-20 11:53:31.000000000 +0200
--- Base.js     2008-09-20 13:31:26.000000000 +0200
***************
*** 242,246 ****
--- 242,251 ----
      ***/
      Divmod.Base.addToCallStack(window, "onload", func, true);
  };

+ Divmod.Base.addUnLoadEvent = function(channel) {
+     if (window.attachEvent)
+         window.attachEvent("onunload", function (e) {channel.sendCloseMessage();});
+ };
+
  Divmod.Base.jsonRegistry = Divmod.Base.AdapterRegistry();

Replying to kozneb:

I propose the following fix for this problem:

Index: __init__.js
===================================================================
--- __init__.js	(revision 16788)
+++ __init__.js	(working copy)
@@ -1475,6 +1475,7 @@
             Nevow.Athena.Widget._pageLoaded = true;
             Nevow.Athena.Widget._instantiateWidgets();
             Divmod.debug("widget", "Finished instantiating live widgets");
+            window.document.body.onbeforeunload = window.onbeforeunload;
         });
 };

Reasoning: In function Nevow.Athena.bootstrap() the property window.onbeforeunload gets assigned a callstack with the call to bindEvents(window). Odd though that the MS debuggers show the property still as null in IE when stopping after bindEvents. The post on bustedmug http://bustedmug.blogspot.com/2007/01/onbeforeunload-ie7-assigning-event.html and some debugging revealed that at least under IE7 window.onbeforeunload cannot be assigned to until window.document.body exists.

Adding the really odd assigment

window.document.body.onbeforeunload = window.onbeforeunload;

in the function transportStartup sets up everything perfectly for the IE browsers, does not disturb the other browsers tested, triggers the onbeforeunload reliably and refresh ad lib is possible with all tested browsers given down below, the lockups on the IE family are gone.

This was tested with the calculator sample on:

• Firefox 2.0.0.14
• Firefox 3.03
• Chrome Build 2200
• Safari 3.1.2
• Opera 9.24
• IE6.0.2800
• IE7.0.5730
• IE8.0.6001 (Beta)

(In [16811]) Branching to 'ie-onbeforeunload-2731'

I've implemented a fix for this, based on wthie's suggested fix; it seems like setting window.onbeforeunload in IE never works, no matter when you do it. I haven't yet tested the fix in IE 6, but I've confirmed that it works in IE 7, and that it doesn't break anything in Firefox 3. Testing in other browsers would be welcome.

Replying to mithrandi: As per your request by eMail I tested branch 'ie-onbeforeunload-2731' with all the browsers given in my comment before with no adverse effects in non IE browsers and reload working as advertised in the IE family now too. The Firefox browsers were tested on MSW only.
Just wondering... I assume that this manual testing is as much as can be provided or does anybody see a way to do this in an automated way

Replying to mithrandi: setting window.onbeforeunload in IE can be done, but only after window.document.body exists in the DOM of the browser which is quite late in the process of loading the page.

wthie, manual testing for this stuff is pretty important since the automated tests are far from comprehensive. However, there are some automated tests. It'd be great if you could run those as well. The runner is broken with current Twisted trunk@HEAD, but works with Twisted 8.1. You can find it at Nevow/bin/nit and the invocation is something like nit nevow. This starts a web server which you can visit in a browser to run the automated tests.

idnar, doesn't moving the call to self.page.bindEvents(window) into transportStartup from bootstrap mean that a user can now hit escape while JavaScript is being loaded and screw up the page? Previously, bindEvents was called very early (from some JavaScript in a <script> tag in the <head>). It probably wasn't impossible to get an escape in before the handler got installed, but it's probably now much easier, particularly for large pages?

(In [16838]) Attach event handlers at different times. Refs #2731

(In [16841]) Unbreak everything. Refs #2731

Okay, good. I think it's weird how bootstrap isn't a method of the page class, since it's basically doing lots of stuff to or with the page, but that's an issue for another day. Please merge this.

(In [16846]) Fix onbeforeunload functionality in IE browsers.

Fixes #2731 Author: mithrandi Reviewer: exarkun

(In [16867]) Branching to 'ie-onbeforeunload-2731-2'

I've fixed the tests now, so this is ready for review again, but I guess #2755 and #2766 should be resolved first before we try to merge this again.

That's #2755 and #2756, of course.

Merge was reverted in r16866

Sorry for the crummy review last time. Here's a better one, I hope:

• Nevow/nevow/js/Nevow/Test/TestWidget.js
  • test_onbeforeunload is a bit weird now. It's half a test for Divmod.Runtime.theRuntime.addBeforeUnloadHandler and half a test for page.makeHandler('onbeforeunload'). A more direct test for the expected functionality would just be to call the result of self.page.makeHandler('onbeforeunload') (or self.page.onbeforeunload? notice the next test calls self.page.onkeypress directly).
• Nevow/nevow/js/Nevow/Test/TestInit.js
  • test_bootstrap no longer covers the installation of the onbeforeunload hook at all

(In [16889]) Just call the handler directly. Refs #2731

(In [16894]) Enhance test coverage of bootstrap process. Refs #2731

I've addressed the two issues mentioned. I removed the Divmod.Base.addLoadEvent override in the Python code for the test harness, because it didn't seem to serve any purpose (the tests pass fine without it), and the right place to do that kind of override is probably in the Spidermonkey runtime class (since Divmod.Runtime.theRuntime.addLoadEvent is presumably the correct way to call that function), not the test harness.

in Nevow/nevow/js/Nevow/Test/TestInit.js, test_bootstrap includes a commented out throw, please delete it.

Otherwise, looks good, please merge.

(In [16924]) Fix Athena onbeforeunload handler registration for IE browsers.

Fixes #2731 Author: mithrandi, wthie Reviewer: exarkun

Due to the onbeforeunload handler not being invoked, Athena transports were not being closed upon navigating away from a LivePage, thus quickly consuming all of the available persistent connection "slots", and causing the browser to be unresponsive to new attempts to navigate to a new page or establish a new Athena transport.

(In [16927]) Revert [16924]. Reopens #2731

(In [16993]) Branching to 'ie-onbeforeunload-2731-3'

Okay, regression fixed in [16996] (hopefully).

1. docs for Spidermonkey runtime's addLoadEvent method are a bit out of sync with the implementation. The loadEvents attribute should be documented as well.
2. in TestInit.js, the comment for test_bootstrap could probably be updated a bit.

Alas, some Blendix tests still fail. I think some code in Blendix should change, but it needs a little more help here. Can you explore how difficult it would be to have a new spidermonkey runtime created for each test method? Without this, any test which invokes code which adds a load event has to clean them up, including tests which don't even know or care that they're invoking such code.

Hm. I suppose another point is that Divmod.Base.addLoadEvent is really just redundant and broken now. It should probably call the runtime's addLoadEvent method now, and be documented as deprecated.

Aside from the fact that Divmod.Base.addLoadEvent is how the other runtimes implement addLoadEvent. Maybe its implementation should move into runtime.

(In [17042]) Reinit runtime before each test run. (refs #2731)

I believe I've dealt with all of the mentioned issues.

1. Nevow/nevow/js/Divmod/Runtime/init.js
   1. A comment in initRuntime about why it's important to cache the runtime type would be good.
   2. Also, nothing ever passes an argument to initRuntime, should it accept one at all?
2. Nevow/nevow/js/Divmod/Test/TestRuntime.js - test_addLoadEvent would probably be better off in a test case class of its own. It's not like the other tests defined in this module, which sort of make sense for other runtimes (although since they're not run against other runtimes I won't go so far as to say that they would pass against other runtimes). Just putting this new test onto a spidermonkey-specific test case would help clarify what's going on, though.
3. Nevow/nevow/js/Divmod/UnitTest.js - run probably isn't the best place for this call to initRuntime, from my current understanding of xUnit. setUp would probably be slightly better, but alas most (all?) subclasses which override setUp do not bother to call the base implementation. Perhaps the best place would be in a generalized extensible fixture API (eg, allow classes to have a fixtures attribute which includes objects with setUp and tearDown methods, all of which are invoked at the appropriate time). Implementing such a thing is certainly out of scope for this ticket, though. Could you file a ticket about how calling initRuntime should be the responsibility of some other code and add a comment near the call pointing to the ticket?
4. Nevow/nevow/js/Nevow/Test/TestInit.js - test_bootstrap goes through the runtime to exercise the keypress and load handlers that get bootstrapped. It doesn't for the onbeforeunload handler, though. It defines a fake onbeforeunload and then it calls it and asserts that it was called. Since the spidermonkey runtime subclasses the base runtime, perhaps the test can call `aWind.onbeforeunload()´ instead?
5. Nevow/nevow/js/Nevow/Test/TestWidget.js - similar comment about the way onbeforeunload is being handled
6. Nevow/nevow/jsutil.py - perhaps generateTestScript's signature will all fit on one line now :)

(In [17044]) Deprecated Divmod.Base.addLoadEvent, and move its implementation into Runtime. (refs #2731)

(In [17068]) Remove initRuntime param and add a comment about caching. (refs #2731)

(In [17069]) Add a comment referencing #2806. (refs #2731)

(In [17071]) Reformat definition. (refs #2731)

1. I've added a comment about the caching behaviour, and removed the argument. The functionality to pass in a specific runtime type may still be useful at some point, but this can wait until there's an actual use case.
2. I've created a new test case for spidermonkey-specific runtime tests, and moved test_addLoadEvent into it.
3. Created #2806, and added a comment referencing it.
4. We can't call aWind.onbeforeunload() because it doesn't exist anymore. The whole point of the Spidermonkey addLoadEvent changes was to avoid actually trying to set attributes on window, so we have to fake it.
5. Same problem here; I have changed the comment on this test to more accurately describe what it does, though.
6. Reformatted the signature.

(In [17070]) Change comment to reflect the test's behaviour more accurately. (refs #2731)

4. addLoadEvent and onbeforeunload are different though :) There's no window.onload but there's still window.onbeforeunload. Possibly the spidermonkey runtime should have a custom implementation of this method as well, but inheriting firefox's doesn't seem harmful for now. So I do think changing the tests (and perhaps leaving a comment about what's going on) makes sense.

I missing something last time, the new Divmod.Base.addLoadEvent calls the runtime's addLoadEvent with no arguments.

(In [17101]) Call aWind.onbeforeunload() (refs #2731)

(In [17102]) Actually pass the param to addLoadEvent(). (refs #2731)

Whoops, guess I hadn't had enough sleep. So, I've changed the way test_bootstrap works; but I think the intention of test_onBeforeUnload is just to test the handler directly, so going through window.onbeforeunload seems like needless complexity.

I've also fixed Divmod.Base.addLoadEvent.

Looks good, please merge.

(In [17111]) Fix Athena onbeforeunload handler registration for IE browsers. (second merge)

Fixes #2731 Author: mithrandi, wthie Reviewer: exarkun

Due to the onbeforeunload handler not being invoked, Athena transports were not being closed upon navigating away from a LivePage, thus quickly consuming all of the available persistent connection "slots", and causing the browser to be unresponsive to new attempts to navigate to a new page or establish a new Athena transport.