Opened 11 years ago

Closed 11 years ago

#6786 closed defect (fixed)

clicking file download link destroys all dijits on page in IE

Reported by: anthony.fryer@… Owned by: James Burke
Priority: blocker Milestone: 1.2
Component: Dijit Version: 1.1b1
Keywords: dijit download file destroy Cc: James Burke, sjmiles
Blocked By: Blocking:

Description

I raised a defect yesterday which was rejected, but i have more details and a better test case now.

The problem is with downloading a file from a page that has any dijits on it. Clicking a file download link is extremely common. For example in gmail, if you download an email attachment you are clicking an href to a url that has a response header with a Content-Disposition of 'attachment; filename="blah.pdf"'. The browser should continue to display the current html page and display a "save or open" dialog to the user.

Alternate ways to download a file are by setting the window.location.href or using an iframe.src. No matter which way you try to download a file the user should have the current page unchanged and a file save or open dialog.

The current svn trunk has a serious problem with downloading files in IE. That is, all dijits on the page are destroyed as soon as you click on a download link (not sure if the destroy() method is called on the dijits..perhaps obliterated is a better word to use to describe what is happening).

I have created a test case that shows dijit.form.Button, dijit.ColorPalette? and dijit.Calender all being destroyed when clicking a download link. Run the attached test case from the dijit/tests/form directory.

Attachments (2)

test_FileDownload.html (5.9 KB) - added by guest 11 years ago.
file download dijit test
DOWNLOADFILE.TXT (1.5 KB) - added by guest 11 years ago.
IFRAME File Download

Download all attachments as: .zip

Change History (17)

Changed 11 years ago by guest

Attachment: test_FileDownload.html added

file download dijit test

comment:1 Changed 11 years ago by dylan

If I had to guess, something is triggering an unload event handler to fire, and the code to destroy widgets, remove event handlers is happening here.

comment:2 Changed 11 years ago by bill

Cc: anthony.fryer@… removed
Reporter: changed from guest to anthony.fryer@…

Ah I see, so even clicking this messes everything up:

<a href="http://download.dojotoolkit.org/release-1.1.1/dojo-release-1.1.1.tar.gz">
Clicking this link will destroy all widgets!</a>

I see your point about the problem.

Hopefully Dylan is right, something w/the code in manager.js:

if(dojo.isIE){
	// Only run this for IE because we think it's only necessary in that case,
	// and because it causes problems on FF.  See bug #3531 for details.
	dojo.addOnUnload(function(){
		dijit.registry.forEach(function(widget){ widget.destroy(); });
	});
}

Is this a new problem in 1.1? Does removing that code help? (Not to imply that the above code is the problem; seems like the unload event shouldn't be firing in the first place...)

comment:3 Changed 11 years ago by bill

Cc: James Burke added

Peter guessed it.... this problem seems to have started with [13555].

Anthony - you are using the latest code rather than the 1.1 (or 1.1.1) release, right?

comment:4 Changed 11 years ago by guest

Huy

I had the exact same problem, I was little nervous when I saw the response to your PR, so made some experiments to find an alternative solution. May be, solution I found, will help until the problem is resolved. It works with both FF2 and IE7

I attached the file

Eduardo

Changed 11 years ago by guest

Attachment: DOWNLOADFILE.TXT added

IFRAME File Download

comment:5 Changed 11 years ago by guest

Yes, I'm using the subversion trunk, so I don't know if this happens in 1.1 or 1.1.1.

Anthony Fryer

comment:6 Changed 11 years ago by guest

I've just implemented this to fix the issue. I haven't tested it extensively but it seems to work. I just changed dojo.uloaded to be called when the "onunload" event happens in IE instead of the "onbeforeunload" event. Here's the svn diff of my change to hostenv_browser.js...

--- hostenv_browser.js  (revision 13766)
+++ hostenv_browser.js  (working copy)
@@ -320,14 +320,18 @@
                                );
                        }

+                       // FIXME: dojo.unloaded requires dojo scope, so using anon function wrapper.

+                       _handleNodeEvent("onunload", function() { dojo.unloaded(); });
+
                        try{
                                document.namespaces.add("v","urn:schemas-microsoft-com:vml");
                                document.createStyleSheet().addRule("v\\:*", "behavior:url(#default#
VML)");
                        }catch(e){}
+               } else {
+                       // FIXME: dojo.unloaded requires dojo scope, so using anon function wrapper.

+                       _handleNodeEvent("onbeforeunload", function() { dojo.unloaded(); });
                }

-               // FIXME: dojo.unloaded requires dojo scope, so using anon function wrapper.
-               _handleNodeEvent("onbeforeunload", function() { dojo.unloaded(); });
        })();

        /*

comment:7 Changed 11 years ago by bill

BTW, related tickets are #6739 and #6784.

comment:8 Changed 11 years ago by James Burke

Cc: sjmiles added

IIRC, "onunload" is not a viable solution since the DOM may not be available and some unload handlers that need to interact with the DOM? I believe Scott Miles had a specific issue with using onunload. It may have been specific to Firefox, but I can see it being a general issue. I'll cc Scott to see if he has any input (if he is available at the moment, which I expect he is not).

I am also surprised in the behavior, that onunload does not fire, but onbeforeunload does fire when following a link that turns out not to destroy the page. Seems like a bug with IE. I wonder how it works if you do a window.location call vs. clicking on a link.

This needs a bit more investigation. I hope to do more this weekend.

comment:9 Changed 11 years ago by James Burke

To test the behavior in general for dojo.addOnUnload and A tags that download files, I modified dojo/tests/_base/_loader/addLoadEvents.html to add an A tag with an href to the tar.gz download of Dojo. You can try the modified test here: http://jburke.dojotoolkit.org/temp/addLoadEvents.html

In all our supported browsers, Safari 3.1.1, Firefox 2.0.0.14, FF 3 RC1, IE 6, IE 7, the addOnUnload callback is triggered when the link is clicked. This means all browsers trigger beforeunload events even if the link navigation does not end up wiping out the page.

(Opera 9.5 Alpha did not trigger beforeunload, in the link or the normal page unload/refresh case, so seems like it is generally broken)

Capturing the anchor click, doing dojo.stopEvent then doing a window.location call also triggered beforeunload callbacks -- same behavior as a plain A tag href.

However, the window.unload event is not triggered in all browsers when the link is clicked. You can see this by using the above test page and if you do not see an alert for "true unload called via dojo.connect", then window unload was not triggered. Contrast that with reloading the page, which does trigger the unload event.

So I think the change in r13555 just uncovered an issue that has been there for a while with dojo.addOnUnload binding to beforeunload.

Dojo/Dijit/Dojox? use dojo.addOnUnload in the following files:

dijit/_base/manager.js: destroys widgets (only done for IE browsers) dijit/_Templated.js: destroys template cache DOM nodes, only for IE browsers.

dojo/_base/html.js: to set _destroyContainer to null, to prevent an IE leak. dojo/_base/xhr.js: cancels all inflight IO requests (could be XHR, script or iframe calls). This was done for IE, related to trac #2357 dojo/_firebug/firebug.js: Clears JS variables holding on to DOM elements, presumably to prevent IE leaks.

dojox/analytics/_base.js: Calls pushData, I think to send out the last analytics collection before page closes. Most likely *needs* beforeunload so it can still use the DOM. dojox/av/_base/flash.js: some cleanup for IE dojox/embed/Flash.js: same cleanup as av/_base/flash.js for IE dojox/cometd/_base.js: to send comet disconnect message. Most likely *needs* beforeunload so it can use the DOM to send message (in dojo.io.script case). dojox/layout/ContentPane.js: allows users of this contentpane to register events via dojo.addOnUnload.

So, most usage (except the comet and analytics modules) are using dojo.addOnUnlaod to clean up IE's mess. Perhaps for those cases, then use the real onunload event, but we need to carefully scrub those unload callbacks: Using the DOM is problematic in unload. If we are just unhooking things from DOM elements our code is referencing, then probably OK, but it would need lots of testing to see if it still leaks (maybe IE marks the DOM node invalid/gone before our cleanup runs, so the cleanup fails, but the leak persists?)

The other problem is API change. It seems like we would need a dojo.addOnBeforeUnload(), and switch analytics and comet to that one, and switch dojo.addOnUnload to window.unload. But that change will likely break some people, if they are assuming dojo.addOnUnload means beforeunload. Yuck.

The other option is to leave dojo.addOnUnload pointing to window.beforeunload and let the code above use dojo.connect(window, "unload", ....) to do the IE cleanup. That would avoid the API change.

I would prefer to change the API, and add dojo.addOnBeforeUnload since that maps better to the underlying browser APIs better, but this will likely need more input from others first. I will bring it up in the weekly contributor IRC meeting.

As a side note: to work around the issue today, route the A tag to target an iframe or a new window when pointing to a file that needs to be downloaded.

comment:10 Changed 11 years ago by Douglas Hays

Milestone: 1.2

comment:11 Changed 11 years ago by dante

this is also visible in test_Dialog.html -- there is a link mid-page that when clicked destory's all the dialogs on the page and throws an error, only IE6 ... solution: use a button, add a hash (#, which unfortunately breaks the scrollIntoView code I think).

comment:12 Changed 11 years ago by dante

(In [14278]) refs #6786 - the link in the test causes all the dialogs to be destroyed. Adding in dojo.connect() workaround, needing an e.preventDefault() call to stop the onunload triggering. only breaks ie6.

comment:13 Changed 11 years ago by dante

(In [14279]) back out the dojo.connect and show to properly use a degradable link to trigger a dialog (as a pseudo-best practice) this is to workaround onUnLoad in ie6 - refs #6786

comment:14 Changed 11 years ago by James Burke

Owner: set to James Burke

comment:15 Changed 11 years ago by James Burke

Resolution: fixed
Status: newclosed

(In [14344]) Fixes #6786. Introduces dojo.addOnWindowOnload and ties IE leak cleanup to that method. Allows file download links and javascript links to work in a page without destroying widgets. \!strict

Note: See TracTickets for help on using tickets.