tejacques / crosstab

A utility library for cross-tab communication using localStorage.
Apache License 2.0
364 stars 58 forks source link

IE11 issues that seem to be unsolvable #32

Closed alias-mac closed 8 years ago

alias-mac commented 9 years ago

@tejacques, sorry to bother you again, but we just found the most weird/bizarre issue in IE...

With a piece of code as simple as this, running in the body/head (not on document ready): http://jsfiddle.net/Ls3fbefj/1/

IE11 doesn't execute the unload function, only the beforeunload when refreshing while still loading (again hitting that sweet spot, since this doesn't happen all the time).

See the highlighted part.

I'm not sure how this can be solved, since beforeunload is a cancelable event (like I explained in https://github.com/tejacques/crosstab/pull/31#issuecomment-113064771) we wouldn't be able to resume the keepaliveLoop.

Two things came to mind:

Solution 1:

The crosstab beforeunload function does what we are doing (canceling any setTimeout loop that might be executed at the end of the program stack) and at the bottom of the function we try to resume it in the next TAB_TIMEOUT like:

// code that is in beforeunload

// try to resume after `TAB_KEEPALIVE` time if the tab is still alive...
window.setTimeout(function() {
    crosstab.stopKeepalive = false;
    keepaliveLoop();
}, TAB_KEEPALIVE);

I believe this solution will be problematic if the user takes TAB_KEEPALIVE time to say yes or no in the confirmation box of the example I posted in https://github.com/tejacques/crosstab/pull/31#issuecomment-113064771 and I'm not sure here if the code will execute or not (at least consistently against all modern browsers).

Solution 2:

We only start crosstab on document ready, wrapping the code near https://github.com/alias-mac/crosstab/blob/18ef65b8c471eb1bc5f634f73d700c3583f3af15/src/crosstab.js#L687:

document.addEventListener("DOMContentLoaded", function(event) { 
  // ---- Setup Storage Listener
  // the rest of the code
});

We need to check if this would work normally on all browsers and if IE really executes the unload if the DOM is reported to be loaded (need to run some tests if you decide to go this way).

Most the browsers also support this on document ready: http://caniuse.com/#search=DOMContentLoaded

There is still one problem with the frozen tabs even with any of the solutions posted above, when the browser crashes. If the browser crashes, the localStorage is maintained in it's last state (unless you have IE11 again and some weird settings - see http://stackoverflow.com/questions/24489542/localstorage-resets-in-ie11-after-closing-browser). In this event, how will we recover from frozen tab? Because we will try to ping the master tab that is gone (it was the window/tab that crashed).

Let me know which solution you think it is better or if we should drop frozen tab and find a different solution (like fallback to take over as the master tab and just resume everything - this will basically mean that if crosstab isn't supported, it will assume it is master all the time and broadcast to itself, right?).

tejacques commented 9 years ago

Thanks for the continued thorough testing. The DOMContentLoaded approach can potentially add a significant latency, which I'd rather avoid if possible. This is especially apparent in requests that use tricks like flushing the head to load and execute scripts while generating the page.

This will require a lot of specific testing, but a dual strategy might work which looks like this:

script execution: start keepalive loop beforeunload: stop keepalive loop, do cleanup, and add a listener on DOMContentLoaded that restarts the keepalive loop, and repeats any setup. unload: stop keepalive loop / perform any necessary cleanup. DOMContentLoaded: remove the beforeunload listener

This seems to work on IE11 in a simple test case:

<!doctype html>
<html>
    <head>
        <title>Testing</title>
        <script>
            window.addEventListener('DOMContentLoaded', function () {
                console.log('DOMContentLoaded');
            });
            window.addEventListener('beforeunload', function () {
                console.log('beforeunload - cancel refresh loop');
                window.addEventListener('DOMContentLoaded', function () {
                    console.log('restart refresh loop');
                });
                return "Are you sure?";
            });
            window.addEventListener('unload', function () {
                console.log('unload - cancel refresh loop');
            });
        </script>
    </head>
    <body>
        <script src="//path/to/a/script/that/loads/really/slow.js"></script>
    </body>
</html>

Here's the chain I am seeing:

If the page is left before the DOM content is loaded, only the beforeunload event fires. If the page fully loads before it is left, beforeunload, and unload fire. If the page is left before the content is loaded, but is cancelled, the DOMContentLoaded event will fire once the page finishes loading.

I never saw a DOMContentLoaded fire after a page was attempted to be closed, and confirmed to close, so it would not re-start the keepalive loop unless it should.

I believe setting it up as described above should work, but requires more testing on various browsers.

tejacques commented 9 years ago

As for your second question about frozen tabs -- I've been thinking about the proper solution for quite some time, but I think I've finally settles on the following:

When a tab starts up, assume (but do not announce) that it is master, and trigger the become master event locally, at the same time broadcast a ping to all other tabs listening asking for the current state. All tabs respond with a pong with their id as well as the id they believe is master. The newly started tab demotes itself from master as necessary.

The upside is that there is no longer any frozen tab state. If a tab is isolated, it simply believes it is master, and continues along happily. The downside is that it will attempt to do any connection or master tab setup every time a new tab is opened, and downgrading from master becomes a necessary component to handle. Another downside is that because it doesn't know who the master should be and initial requests the tab may have of the master will have some additional latency added. That said, it seems like the best overall approach as it would allow removing any mobile specific restrictions as well as the entire concept of a frozen tab.

alias-mac commented 9 years ago

@tejacques, thanks for the fast reply. I'll do some testing this weekend.

For your second comment about the frozen tab, I think the problem is that if the tab promotes itself to be the master, then it might slow things down. E.g.: if we are doing a socket connection every time a tab is promoted to be the master, that means that each tab (every time it opens) will have to do that connection and the other tab that was demoted will have to disconnect. Other logic might apply (when the tab becomes the master) that can have some performance degradation.

May I suggest to apply that logic that you said about believing that the tab is the master and notify everyone else, only when a timeout occurs? This would allow the reuse of the master tab if it replies on time, and promote itself if the master is too busy and doesn't reply (either on time or never). The master will have 2 messages in the queue, the PING and the tab (now) promoted. We already handle the fact that a PONG doesn't make sense to be sent (it is too late to reply): https://github.com/tejacques/crosstab/blob/master/src/crosstab.js#L705

This means that during the setup (until setupComplete event), which is the most likely to trigger a frozen tab detected event, we would take over any master tab that is no longer active and wasn't able to communicate that properly (due to crash or any other problem, like the IE11 not notifying the other tabs on unload). It also handles the problem of only one of the tabs crashed and not the entire browser (in this particular case, the master tab crashed), then one of the tabs that is trying to broadcast to master and master isn't replying (after timeout), will automatically notify that master is dead (like we currently do in var deadTabs = util.filter(util.tabs, notAlive);).

Potencial problems: If the 2 tabs ping the master at the same time and the timeout occurs at exactly same time, can potentially try to notify that master is dead twice. At this point, I think we can rely on the fact that it is single threaded, so 2 messages are going to be saved in localStorage and both with be read consecutively. I think this is no different from the current implementation of deadTabs, since we are triggering duplicated tabClosed events with the same tab id, if 2 tabs executed that keepalive part of var deadTabs = util.filter(util.tabs, notAlive);) at the same time (each tab is single threaded, but they run that same code concurrently and send messages about those tabs). I'm not sure if this is by design or if if the deadTabs code was meant to be executed by the master tab only (since it is the one that will - at the end - write those tabs).

I'll make a PR soon with what I have in mind so we can further discuss.

tejacques commented 9 years ago

The tradeoff there is also latency. The current design reads the master tab info from localStorage, and PINGs, however it skips this step if it believes it does not have a frozen tab environment (I.E. Some tab successfully had a handshake with the master tab on this domain). This means the price of that handshake is paid only once. If the frozen tab state is removed and each tab tries to handshake every time it starts up, this adds an appreciable amount of latency, especially in the case where the handshake fails because this is the first tab. The main problems with the current frozen tab state design is that mobile does not work at all (even though it could benefit from iframes), and there is no recovery attempt. Removing the state would allow all platforms to behave similarly. There may also be a middle ground here where we can limit the scenarios it needs to handshake, such as repurposing the frozen tab state to mean that it should PING but assume it will fail and behave as I described, but if it is not set it PINGs and assumes it will succeed.

Typed this on my phone so hopefully it's clear enough. This particular part is probably the most complicated of the entire library, so getting it right will be tricky.

tejacques commented 8 years ago

@alias-mac Are you still interested in adding this? (proper unload handling). I've verified that the outlined solution should work on all supported browsers -- I can add it in this week but wanted to make sure you're not working on it already.

alias-mac commented 8 years ago

@tejacques we are working on it, and our QA is testing across all browsers. It seems that this solution (with the beforeunload that you proposed) is still causing some issues on iPad ("Mozilla/5.0 (iPad; CPU OS 8_4 like Mac OS X) AppleWebKit/600.1.4 (KHTML, like Gecko) Version/8.0 Mobile/12H143 Safari/600.1.4"). We are trying to see what the problem might be.

I'll send out a PR in a few hours with the changes we have, but it seems that with this problem on the iPad we need more time to bake a solution for this.

alias-mac commented 8 years ago

@tejacques it seems that crosstab.supported says that it is true on iPad but it isn't, because none of the beforeunload or unload events are executed. Can you confirm which browsers you support on this lib?

alias-mac commented 8 years ago

@tejacques, sorry for the spam. One more update. We were trying to solve this with pagehide event that exists on iOS devices, but that (like IE11) doesn't trigger unless the dom was fully loaded, meaning that we can't dispose crosstab correctly unless we only start it on DOMContentLoaded.

Do you plan on adding iOS safari devices to the isMobile, so that crosstab.supported is false in this case? Based on our previous comments (above) I really don't see how we can avoid this frozen tab env problem without taking over the master tab when it is unresponsive.

alias-mac commented 8 years ago

Posting here the code that I'm using to test this:

<!DOCTYPE html>
<html>

<head>
<script type="text/javascript" src="path/to/crosstab/src/crosstab.js"></script>
<script type="text/javascript">
    window.addEventListener('beforeunload', function (e) {
        console.log('beforeunload - cancel refresh loop');
        var confirmationMessage = "Are you sure?";
        (e || window.event).returnValue = confirmationMessage;
        return confirmationMessage;
    }, false);
    console.log('events added');
</script>
</head>

<body>
    <script type="text/javascript" src="path/to/script/with/10/sec/sleep"></script>
</body>
tejacques commented 8 years ago

When I tested it was on the browsers listed on the readme (desktop only). All mobile type browsers (tablets/phones/etc.) should currently have crosstab as not supported. If some browsers like safari iPad aren't caught in the isMobile check, that should be updatedable to . Let's open a separate issue to tackle that.

In version 1.0.0 the goal is to have a graceful fallback to local-tab-only broadcasted events when broadcasting to other tabs is unavailable, but broadcasts should work on mobile across iframes in the same tab.

alias-mac commented 8 years ago

@tejacques chrome on android is working though (at least it seems so). In that case I'm going to update this PR to remove the console.log() and the iPad pagehide support and wait for you to bump the version. Thanks!

tejacques commented 8 years ago

There's nothing inherent in any of the mobile browsers on a single page that make this not work -- it's the behavior when you have multiple tabs open on mobile browsers that break it, since JavaScript will only run on the active tab. Detecting those and setting frozenTabEnvironment to true is just a way to say it's not supported at the moment.

alias-mac commented 8 years ago

@tejacques I'm having a bit of trouble stubbing some methods to do some automated testing on this PR. I was thinking about stubbing unload and swapUnloadEvents so I could test the abort use case. Since the setup will already fire a DOMContentLoaded, I won't be able to check that the swap was done by DOMContentLoaded event.

If you are ok on just checking that after DOMContentLoaded all is setup correctly and don't add unit tests for the cancel behavior, then I'll just add what I have to the current PR.

Thanks!

tejacques commented 8 years ago

I'm fine with that. The only way you could test DOMContentLoaded things is by using iframes. I think it's possible but that's a rarer case so I'm not as worried about it for now.

I'm on my way home now, will point out what I changed to avoid the race condition for the double loop.

alias-mac commented 8 years ago

@tejacques cool in that case I just need to know how can I restore the loop so it doesn't blow the next tests. I have the following test:

// first test
window.dispatchEvent(new Event('beforeunload'));
// test that nothing changed and crosstab.stopKeepalive is still false

// second test
// spy on `util.events.destructor`.
window.dispatchEvent(new Event('unload'));
// test that crosstab has stopped running.
// `util.events.destructor` was called.

if the tests are added in the bottom (and kept there) there is no problem (I think), but if we add more tests after these, crosstab broadcast messages will never run. Are ok on keeping the bottom tests to be always tested after crosstab is destroyed? If not, can I make restoreLoop to be public (specially after your suggested change that will make sure that even if called multiple times it will always avoid N loops)?

tejacques commented 8 years ago

Here's the commit: 03f7d0dcb8530e08ef5eedb51f29bb0599e3523b

Basically, if you call keepaliveLoop() now it will just do a keepalive, if crosstab.supported && !crosstab.stopKeepalive. It won't spin up a new loop - there's just one persistent loop.

tejacques commented 8 years ago

Ideally all of the tests should be able to run in any order. There's two ways around it:

  1. You can just set crosstab.stopKeepalive = false, because now with my other change, the loop doesn't actually stop, it just doesn't execute anything when it's false.
  2. You could run these tests inside an iframe. You can view some of the other tests that do this to see how it's done: https://github.com/tejacques/crosstab/blob/master/test/test-crosstab.js#L198-L228

The second option is safer because the changes are sequestered in their own iframe, but if you do go for the first option, just make sure to set it back to true after the test whether or not it succeeds.

alias-mac commented 8 years ago

@tejacques I added the unit tests and with some console logs around the main functions that we want to track I got:

setup during load
unload
trying to restore - swaping events
running again
      ✓ should stop on beforeunload event and recover when canceled
trying to restore - swaping events
unload
      ✓ should stop on unload event after DOMContentLoaded event

which indicates that all is being tested as expected.

Let me know if you find anything that should be changed.

Thanks again for all the help on this!

tejacques commented 8 years ago

Thanks for putting this together @alias-mac -- I will test tonight and merge if all looks well.

alias-mac commented 8 years ago

@tejacques, let me know when we can have this on the next release version, so we can use it. Thanks.

tejacques commented 8 years ago

@alias-mac: didn't merge yesterday since it was late, but passed all sauce tests and looks good :+1: -- will launch updated version today.

alias-mac commented 8 years ago

Thanks once again for all the help @tejacques. Looking forward to discuss in another ticket and see the possible solution we talked above (https://github.com/tejacques/crosstab/pull/32#issuecomment-115966098) to avoid the frozen tab.

tejacques commented 8 years ago

Thank you for the contributions. This is now live on npm.

I have something I'm putting together for better support/phasing out frozen tabs using PageVisibility on mobile browsers. For browsers that don't have it, they will treat each window/iframe as it's own isolated master.