Chrome 13 Edge-case Bugfix in waitForReady when 'frame' is undefined #120
Conversation
In a particular users Chrome browser (who was using various extensions), 'frame' wasn't defined by the time the waitForReady function was fired, thus causing it's initialization failure. The console was complaining about this immediately after it printed "firing onReady" in the debug output. While it wasn't affecting anyone in our development team, verifying that 'frame' exists first seems like the correct thing to do and fixed this users problem.
|
I cannot see how this is considered a fix since this situation is impossible unless something really fishy is going on... |
|
I agree that it's fishy, but I don't think that means a fix shouldn't be considered. It was happening on 1 out of 8 users computers in our organization, 15% of the time. This means it could be potentially be affecting 2% of users. Bugs like this are annoying. They were running several very popular Chrome extensions. I tried selectively disabling them to narrow the issue but with as infrequently as it was happening, it wasn't worth the effort to figure out which extension it was (even if I narrow it down, I'm not going to go fix the extension, I'm going to fix our code which is used by our end-users). With debug on, I spotted the bug in his console after a day of waiting and made this modification. The problem went away, and that was over a month ago now - so this is definitely a fix. I realize it's a fix for a fishy situation, but if it can be easily accounted for, it should be. This doesn't add complexity to the code, and solves a particular edge-case (which could be happening in other situations). I'm happy to be able to account for it. |
|
Sure, I get this, but my point is that the message I'd rather try to figure out why this message would be sent multiple times, and one time prematurely instead of just covering over it. The real fix would be to make sure that this particular message isn't sent unless it should be sent. As I see it, this code branch, https://github.com/oyvindkinsey/easyXDM/blob/master/src/stack/PostMessageTransport.js#L141, is for some reason entered outside the regular flow, and it is this that must be addressed. |
As shown in pull request oyvindkinsey#120 and currently shown on twitter.com home timeline when scrolling rapidly, the waitForReady function can complain that the frame is null. The probable cause is that the waitForReady method isn't torn down in the destroy method. So if an iframe is destroyed before it initialises, it can throw this kind of error. twitter.com: `Uncaught TypeError: Cannot read property 'contentWindow' of null` I've pulled out the function and added an unbind. I think this makes sense. (Edited through github editor - needs verification/testing)
As shown in pull request oyvindkinsey/easyXDM#120 and currently shown on twitter.com home timeline when scrolling rapidly, the waitForReady function can complain that the frame is null. The probable cause is that the waitForReady method isn't torn down in the destroy method. So if an iframe is destroyed before it initialises, it can throw this kind of error. twitter.com: `Uncaught TypeError: Cannot read property 'contentWindow' of null` I've pulled out the function and added an unbind. I think this makes sense.
In a particular users Chrome browser (who was using various extensions), 'frame' wasn't defined by the time the waitForReady function was fired, thus causing it's initialization failure. The console was complaining about this immediately after it printed "firing onReady" in the debug output. While it wasn't affecting anyone in our development team, verifying that 'frame' exists first seems like the correct thing to do and fixed this users problem.