Opened 10 years ago

Closed 10 years ago

Last modified 7 years ago

#10095 closed defect (fixed)

[patch][ccla]IE: dojo.destroy() can fail

Reported by: Douglas Hays Owned by: Douglas Hays
Priority: high Milestone: 1.4
Component: Core Version: 1.4.0b
Keywords: Cc:
Blocked By: Blocking:

Description (last modified by Douglas Hays)

Run the attached testcase in any IE.
1) click the button to create a widget in the main window.
2) click the button to create a widget in the iframe.
3) click the button to destroy the iframe wiget.
4) click the button to reload the iframe.
5) repeat steps 2 and 3.
6) On IE6, press the browser's HOME button and the browser locks up.
At this point the destroy has failed and the widget is still in the iframe.
The problem is that the new iframe document is not permitted to look at nodes from the first iframe's document, but dojo.destroy does:

_destroyContainer.ownerDocument != node.ownerDocument

which throws and exception and then causes an infinite loop in IE when the main window is unloaded.
The fix is to save the ownerDocument in a local variable and access that instead.

Attachments (5)

destroybug.html (1.0 KB) - added by Douglas Hays 10 years ago.
test file
iframesrc.html (144 bytes) - added by Douglas Hays 10 years ago.
test iframe source file
10095.patch (1013 bytes) - added by Douglas Hays 10 years ago.
possible fix to be reviewed
testDestroy.html (2.2 KB) - added by Douglas Hays 10 years ago.
benchmark test file showing much faster destroy() for IE
10095alt.patch (1.2 KB) - added by Douglas Hays 10 years ago.
alternate patch using outerHTML for IE

Download all attachments as: .zip

Change History (16)

Changed 10 years ago by Douglas Hays

Attachment: destroybug.html added

test file

Changed 10 years ago by Douglas Hays

Attachment: iframesrc.html added

test iframe source file

Changed 10 years ago by Douglas Hays

Attachment: 10095.patch added

possible fix to be reviewed

comment:1 Changed 10 years ago by Douglas Hays

Summary: IE: dojo.destroy() can fail on IE[patch][ccla]IE: dojo.destroy() can fail

comment:2 Changed 10 years ago by James Burke

Owner: changed from James Burke to Douglas Hays

Feel free to apply the patch, but I would not use _destroyDoc, but rather just use _destroyContainer.ownerDocument in the if test.

comment:3 Changed 10 years ago by Douglas Hays

Using _destroyContainer.ownerDocument is actually the code that is throwing the exception.

comment:4 Changed 10 years ago by Douglas Hays

Milestone: tbd1.4
Status: newassigned

comment:5 Changed 10 years ago by Douglas Hays

Description: modified (diff)

comment:6 Changed 10 years ago by James Burke

Oops, yeah, forgot to read the stuff at top, but this gets into Dojo supporting using dojo in another iframe, which in today's meeting we said we would not explicitly support. I am reluctant to keep around the document of possibly another frame in the main dojo window. it seems like an introduction for leaky or bad behavior.

I am now tending to say this would be a wontfix.

comment:7 Changed 10 years ago by Douglas Hays

jburke, I committed the change to [20576] before I saw the previous comment. I'll gladly back it out if you want. The actual revision does not really add any functionality but just caches the ownerDocument value, which just happens to make things work better with iframes, and is probably more performant anyway.

Changed 10 years ago by Douglas Hays

Attachment: testDestroy.html added

benchmark test file showing much faster destroy() for IE

Changed 10 years ago by Douglas Hays

Attachment: 10095alt.patch added

alternate patch using outerHTML for IE

comment:8 Changed 10 years ago by Douglas Hays

jburke, while I still like [20576], I attached an alternate patch for your review that just sets outerHTML= for IE. Performance-wise, this way takes 1/2 the time of the previous destroy method and still allows other browsers to access _destroyContainer.ownerDocument if that's what you prefer (even if that references a nonexistent document and probably should throw an exception). Ideally, I think the correct fix would be to keep [20576] AND add the IE outerHTML change for performance. I tested the outerHTML change and there were no leaks as reported by sIEve using the attached benchmark test.

comment:9 Changed 10 years ago by James Burke

Yeah, sorry about the late comment. If you did a leak check and it looks OK, then we can keep the [20576] change in there, but let's not do the isIE branch for outerHTML. I though we used outerHTML at some point, but there was still a problem with it, IIRC. Also, I want to avoid the extra code in base, as well as a browser detect branch.

comment:10 Changed 10 years ago by Douglas Hays

Resolution: fixed
Status: assignedclosed

Fixed in [20576]. Cached ownerDocument in dojo.destroy to prevent attribute access that can throw an exception in IE. Added automated test to html.html.

comment:11 Changed 7 years ago by bill

See #12342, this test is failing for IE9+. It should be fixed or disabled for IE9+ or maybe just removed; this might be an exceptional case where having the test is more trouble than it's worth.

Last edited 7 years ago by bill (previous) (diff)
Note: See TracTickets for help on using tickets.