Bug Tracker

Modify

Ticket #10080 (closed bug: fixed)

Opened 3 years ago

Last modified 3 years ago

unload from frame's window breaks in IE8

Reported by: brian.moschel@… Owned by: rwaldron
Priority: low Milestone: 1.6.3
Component: data Version: git
Keywords: Cc:
Blocking: Blocked by:

Description

If you have an iframe in the same domain and try to bind to unload:

$(someotherwindow).unload(fn)

This works in all browsers except IE8.

 http://jsfiddle.net/Y4Txu/1/

The problem is when you bind to unload, it uses the "one" method, so after unload, it unbinds itself and calls jquery.removeData, which has this:

// Browsers that fail expando deletion also refuse to delete expandos on
// the window, but it will allow it on all other JS objects; other browsers
// don't care
if ( jQuery.support.deleteExpando || cache != window ) {
  delete cache[ id ];
} else {
  cache[ id ] = null;
}

That first if is true for IE8 and below I believe. Cache isn't the current window in this case, its another frame's window object, so delete cache[id] throws an error, and the unload event handler isn't reached

The fix is to check if cache is any window, not just the current window

Change History

comment:1 Changed 3 years ago by anonymous

I should say the error thrown is "Object doesn't support this action" and at least in 1.6.2 its on delete cache[id].

The error prevents the event handler from being called at all.

comment:2 Changed 3 years ago by rwaldron

  • Priority changed from undecided to low
  • Resolution set to duplicate
  • Status changed from new to closed
  • Component changed from unfiled to data
  • Milestone changed from None to 1.6.3

comment:3 Changed 3 years ago by rwaldron

Duplicate of #8235.

comment:4 Changed 3 years ago by brian.moschel@…

I don't think those 2 bugs are duplicates. In 8235, they said:

cache[ id ][ internalKey ] is checked without ensuring that cache[ id ] exists.

In the error I'm describing, the problem is

if ( jQuery.support.deleteExpando
cache != window )

is true if cache is another frame's window, but delete cache[id] breaks. That if statement should check if cache is ANY window, not just the current one. Those are not the same cases.

comment:5 Changed 3 years ago by brian.moschel@…

That code sample should show:

if ( jQuery.support.deleteExpando || cache != window )

comment:6 Changed 3 years ago by rwaldron

  • Status changed from closed to reopened
  • Resolution duplicate deleted

Or you could simply ensure that cache[ id ] actually exists...

Which is the same issue.

 https://github.com/jquery/jquery/pull/468

comment:7 Changed 3 years ago by rwaldron

comment:8 Changed 3 years ago by dmethvin

Is this a result of not having a test case for #8235? We fixed that problem without knowing the cause.

comment:9 Changed 3 years ago by brian.moschel@…

That pull request doesn't actually address the issue I'm talking about. The core issue is that cache can be any window object.

I made a pull request with a test that currently breaks in IE8 and a fix.

 https://github.com/jquery/jquery/pull/470

comment:10 Changed 3 years ago by rwaldron

@dmethvin, no they are actually unrelated.

comment:11 Changed 3 years ago by rwaldron

Using jQuery.isWindow will be significantly slower and all calls to removeData will be affected, see:  http://jsperf.com/iswindow-vs-inline-test

I've updated my branch to test for a window inference and have included your test with credit to you.

comment:12 Changed 3 years ago by dmethvin

Yes .isWindow() is slower but look at the numbers. Even the "slow" path can run almost 2 *million* times a second. It's not likely the .removeData() path is critical enough that the difference matters.

comment:13 Changed 3 years ago by dmethvin

  • Owner set to rwaldron
  • Status changed from reopened to assigned

comment:14 Changed 3 years ago by Dave Methvin

  • Status changed from assigned to closed
  • Resolution set to fixed

Merge pull request #468 from rwldrn/10080

Fixes #10080. Test cache for window inference.

Changeset: f4811bfb04206b9d9c120001603eadb1b529c271

comment:15 Changed 3 years ago by dmethvin

#8870 is a duplicate of this ticket.

Please follow the  bug reporting guidlines and use  jsFiddle when providing test cases and demonstrations instead of pasting the code in the ticket.

View

Add a comment

Modify Ticket

Action
as closed
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.