196317 - [AxPlugin] Ofoto drag and drop control doesn't work in plugin

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: APIs, defect)

Description

[Adam Lock]

The Ofoto.com website offers several ways to upload files to its server. One of
these is called "Drag and Drop Upload". This page hosts a simple control which
acts as a drop target that the user can drag and drop files onto for automatic
upload. After dragging and dropping, the supplied files are uploaded via a popup
window and a form submit to the server.

Problem:

The control loads and displays fine in Mozilla using the activex plugin, however
dragging and dropping does nothing.


Steps for reproducing behaviour in Mozilla:

1. Run IE and log into ofoto.com as normal, go to the Add Pictures area, select
an Album and choose Drag and Drop Upload

2. Copy the URL for this page and paste it into Mozilla

This circumvents sniffer code in the site which prevents Mozilla users from even
being offered the Drag and Drop Upload option.


Analysis:

The ofoto control works like this. 

1. The page contains a number of javascript routines that automatically upload
arrays of pictures. It also contains an OBJECT tag that creates the simpl
control and instructions on how to use it.

2. When the user drops files over control it calls the plugin's
IHTMLWindow2::execScript implementation with a script of this form:

var pictures = new Array(1);
pictures[0] = new OfotoPic('c:\\personal\\pictures\\test.jpg', 36269, 300, 460);
dropPictures(pictures);

The dropPictures function is in the page content and adds the array of pictures
(which could be more than 1 depending on how many were dropped to a list for upload.

3. This is followed by another call to IHTMLWindow2::execScript with the script.

dropFinished()

The dropFinished function is in the page content and starts the actual upload.

Since the plugin's implementation of IHTMLWindow2::execScript does nothing at
all, none of this actually happens. To fix this problem, the implementation of
execScript must actually run the script in the window.

The actual JS content of the page looks generic, so there is a good chance it
would work if this function can be implemented.

Comment 1

[Adam Lock]

It seems that I should wire up execScript along similar lines as the jsurl
implementation.

http://lxr.mozilla.org/seamonkey/source/dom/src/jsurl/nsJSProtocolHandler.cpp#116

I don't see any code I can utilise so I might have to duplicate it.

Comment 2

[Adam Lock]

Attachment: Work in progress

Patch implements execScript. Still may need work since the script executes but
doesn't appear to trigger the proper behaviour.

Comment 3

[Adam Lock]

Attachment: Patch

Clean patch.

I have verified that this implementation executes the script supplied by the
control, which in turns executes a function in the page content. Unfortunately
a function in the Ofoto drag and drop upload content called
openProgressWindow() attempts to call the named control "DndCtrl" directly
rather than by "document.DndCtrl" and this prevents the script from executing
any further. If I hack the patch so that "var DndCtrl = document.DndCtrl;" is
prepended to the executed script, it gets a bit further and opens the uploading
popup before hitting some other JS errors.

Anyway, the existing issues boil down to evangelism / content problems on the
page. I'm satisfied that the execScript functionality is working at least.

Comment 4

[Adam Lock]

Comment on attachment 116854 [details] [diff] [review]
Patch

Dave & Alec, can I have an r/sr on this please? Thanks

Comment 5

[Alec Flett]

let's hear from mitch to see what we're supposed to be doing with that principal
- one way or the other, the TODO comment implies that there is some security
checks that might need to go in.. so the comment  should either be removed or
the checks should be added.

Comment 6

[Adam Lock]

As background info, I put the TODO there because I was basing my code of the JS
url handler, and it invokes the script security manager:

http://lxr.mozilla.org/seamonkey/source/dom/src/jsurl/nsJSProtocolHandler.cpp#206

Now my code executes in the context of the window it is loaded by, but perhaps
there are reasons that we want to stop it calling out anyway. 

Comment 7

[David Bradley]

I noticed the call to EvaluateString, you're passing in bIsUndefined, but you
never use it. I filed bug 196895. Most of the code checks the pointer value,
allowing for it to be null. However the first use, doesn't. Not really a big
deal for this
bug.

I'll wait for Mitch's comments as well. But other wise I'm ok with it.

Comment 8

[David Bradley]

jst, just checked in the fix for EvaluateString call, so you should be able to
remove the bIsUndefined. Not a big deal, up to you.

Comment 9

[Mitchell Stoltz (not reading bugmail)]

Adam,
   There are a couple more things you need to do before calling EvaluateString.
In order for the security manager to work right while the script is running, you
need to make sure the current JS context is on the JS context stack. Also, you
need to call nsIScriptSecurityManager::CanExecuteScripts to see if JavaScript
has been disabled. You can see an example of both of these at
http://lxr.mozilla.org/seamonkey/source/js/src/liveconnect/nsCLiveconnect.cpp#144.

I don't think you need to copy the code for adding a dummy frame to the JS stack
(lines 193-227 in the above file) - EvaluateString should take care of that
part. Please run these tests:

1. Disable JavaScript in Preferences->Advanced->Scripts & Plugins. Controls
should not be able to run JS when it is disabled there.

2. Have a control try to run this script:
alert(Components.classes);
You should _not_ see an alert; but a permission-denied error in the JS console
instead. This proves the JS is not being given special privileges.

Comment 10

[Alec Flett]

Comment on attachment 116854 [details] [diff] [review]
Patch

(clearing review requests)

Comment 11

[Adam Lock]

I think EvaluateString takes care of the stack, or at least that is my
understanding from step debugging it:

http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsJSEnvironment.cpp#669

It also checks if JS is disabled

http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsJSEnvironment.cpp#626

I shall run tests anyway to see what happens when JS is disabled and what
privileges the script runs with.

Comment 12

[Adam Lock]

Attachment: Patch

Updated patch removes the TODO comment and passes nsnull instead of
bIsUndefined.

I have verified that if I prepend "alert(Components.classes);" to the script
run by the control that it throws an exception. I have also verified that
disabling JS prevents the script from executing. The EvaluateString
implementation takes care of pushing the JS context onto the stack and testing
if JS is enabled.

I think this is ready for r/sr.

Comment 13

[David Bradley]

Comment on attachment 116972 [details] [diff] [review]
Patch

r=dbradley

Comment 14

[Mitchell Stoltz (not reading bugmail)]

Adam,
   You're right, nsJSEnvironment takes care of the stack and the
CanExecuteScripts call. I forgot about that.

Comment 15

[Adam Lock]

Comment on attachment 116972 [details] [diff] [review]
Patch

Hi Alec, can you sr this update please? Thanks.

Comment 16

[Alec Flett]

Comment on attachment 116972 [details] [diff] [review]
Patch

cool, thanks for getting that cleared up. a comment might be helpful explaining
why this is safe/etc
sr=alecf

Comment 17

[Adam Lock]

Thanks all, fix is checked in with an additional note about the script context
taking care of the context stack and js enabled/disabled issues.