118003 - [viewpoint] executing javascript through xpconnect interface won't resolve relative URL

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Ari Berger]

From Bugzilla Helper:
User-Agent: Mozilla/4.0 (compatible; MSIE 5.5; Windows NT 5.0)
BuildID:    0.9.4 branch

I worked with Patrick Beard to get our javascript pipe running through 
xpconnect.  our scriptable interface has a property.  When you set that 
property with a string, it calls eval(string).
It is working great.  One small problem that we have run into is that if you 
try to use a relative url here, it doesn't work.
This content has a cube and a sphere.  Click on the cube and the plugin will 
script out:
window.open('http://cole.viewpoint.com/~aberger/openpopwindow/popup.html',...
and it works great.
Click on the sphere and we script out:
window.open('popup.html',...
and it fails- asserts in your code somewhere, I think because it doesn't have a 
base url to resolve against.
The page also shows that if we do this directly from the html, it (of course) 
works either way.


Reproducible: Always
Steps to Reproduce:
1.Install Viewpoint plugin (see http://cole.viewpoint.com/~aberger/readme.txt)
2.View content http:/cole.viewpoint.com/~aberger/openpopupwindow/index.html
3.Click on the cube- popup will open.  Close the popup
4. Click on the sphere- nothing happens

Expected Results:  sphere and cube should do the same thing.

Obviously a minor problem for us- simple workaround in content.  Just wanted it 
logged.
again, i'm not sure if this is a plugin, javascript, or xpconnect.

Comment 1

[Patrick C. Beard]

This might be a DOM problem rather than any of the components you mentioned. Did
you try to do the window.open() in just a simple HTML document rather than from
the plugin? If it works in that case, then perhaps some context is getting lost
in the call.

Comment 2

[Ari Berger]

> Did you try to do the window.open() in just a simple HTML document rather 
than from the plugin?

The test content has 2 buttons that do just this: one does a window.open with 
the absolute path, one with the relative path.  They both work.
The 2 shapes within the plugin mimic this behavior: One uses absolute, one 
relative.  Only the absolute works.

Comment 3

[Johnny Stenback (:jst)]

Could you show us the stacktrace for the assertion you mentioned?

Comment 4

[Ari Berger]

Assertion is at nsAboutProtocolHandler.cpp line 108:
    // no concept of a relative about url
    NS_ASSERTION(!aBaseURI, "base url passed into about protocol handler");

Call stack:
NTDLL! 77fa018c()
nsDebug::Assertion(const char * 0x013ae858, const char * 0x013ae84c, const char 
* 0x013ae800, int 108) line 290 + 13 bytes
nsAboutProtocolHandler::NewURI(nsAboutProtocolHandler * const 0x0240ec30, const 
char * 0x04541bf0, nsIURI * 0x02411818, nsIURI * * 0x0012cee4) line 108 + 29 
bytes
nsIOService::NewURI(nsIOService * const 0x00e88080, const char * 0x04541bf0, 
nsIURI * 0x02411818, nsIURI * * 0x0012cee4) line 695 + 35 bytes
NS_NewURI(nsIURI * * 0x0012cee4, const char * 0x04541bf0, nsIURI * 0x02411818, 
nsIIOService * 0x00e88080) line 81 + 24 bytes
GlobalWindowImpl::SecurityCheckURL(const char * 0x04541bf0) line 3984 + 44 bytes
GlobalWindowImpl::OpenInternal(GlobalWindowImpl * const 0x03688b38, const 
nsAString & {...}, const nsAString & {...}, const nsAString & {...}, int 0, 
long * 0x00000000, unsigned int 0, nsISupports * 0x00000000, nsIDOMWindow * * 
0x0012d598) line 3208 + 12 bytes
GlobalWindowImpl::Open(GlobalWindowImpl * const 0x03688b40, nsIDOMWindow * * 
0x0012d598) line 2349 + 49 bytes
XPTC_InvokeByIndex(nsISupports * 0x03688b40, unsigned int 13, unsigned int 1, 
nsXPTCVariant * 0x0012d598) line 139
XPCWrappedNative::CallMethod(XPCCallContext & {...}, XPCWrappedNative::CallMode 
CALL_METHOD) line 1952 + 42 bytes
XPC_WN_CallMethod(JSContext * 0x0242dce8, JSObject * 0x035b9cd8, unsigned int 
3, long * 0x04431e40, long * 0x0012d7a0) line 1262 + 14 bytes
js_Invoke(JSContext * 0x0242dce8, unsigned int 3, unsigned int 0) line 807 + 23 
bytes
js_Interpret(JSContext * 0x0242dce8, long * 0x0012e0fc) line 2719 + 15 bytes
js_Execute(JSContext * 0x0242dce8, JSObject * 0x035b9e08, JSScript * 
0x04462ac0, JSStackFrame * 0x0012e940, unsigned int 2, long * 0x0012e0fc) line 
989 + 13 bytes
obj_eval(JSContext * 0x0242dce8, JSObject * 0x035b9cd8, unsigned int 1, long * 
0x04431e20, long * 0x0012e0fc) line 1026 + 37 bytes
js_Invoke(JSContext * 0x0242dce8, unsigned int 1, unsigned int 0) line 807 + 23 
bytes
js_Interpret(JSContext * 0x0242dce8, long * 0x0012e9ac) line 2719 + 15 bytes
js_Invoke(JSContext * 0x0242dce8, unsigned int 1, unsigned int 2) line 824 + 13 
bytes
nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJSClass * const 0x03a3d060, 
nsXPCWrappedJS * 0x03a9f358, unsigned short 3, const nsXPTMethodInfo * 
0x0452c350, nsXPTCMiniVariant * 0x0012eea4) line 1022 + 22 bytes
nsXPCWrappedJS::CallMethod(nsXPCWrappedJS * const 0x03a9f358, unsigned short 3, 
const nsXPTMethodInfo * 0x0452c350, nsXPTCMiniVariant * 0x0012eea4) line 430
PrepareAndDispatch(nsXPTCStubBase * 0x03a9f358, unsigned int 3, unsigned int * 
0x0012ef54, unsigned int * 0x0012ef44) line 100 + 31 bytes
SharedStub() line 124
CPluginInstance::DoScriptEval(const char * 0x00185688, long -1) line 2517
CPluginInstance::ExecuteJavascript(const char * 0x00185688) line 1515
ExecuteJavaScript(const char * 0x00185688, IMtsServices * 0x0454c1ec) line 859 
+ 17 bytes
MTS_HostScript::Execute(IMTS_SymbolTable * 0x04464858, IMTS_Scene * 0x04349890) 
line 634 + 38 bytes
MTS_InterpreterImpl::Execute(unsigned long 11) line 114 + 71 bytes
MTS_DefaultInteractor::Process(unsigned long 0, IMTS_Event * 0x0012f304) line 
247 + 31 bytes
MTS_DefaultInteractor::HandleEvent(IMTS_Event * 0x0012f304) line 292 + 18 bytes
MTS_EventPump::DispatchEvent(IMTS_Event * 0x0012f304, unsigned char 0) line 84 
+ 17 bytes
MTS_Scene::HandleEvent(const CXPEvent * 0x0012f9e8, IPB_Context3d * 0x043c3dc0) 
line 4333 + 20 bytes
CSceneComponent::DoSystemEvent(CXPEvent & {...}) line 2857 + 47 bytes
CGenieControl::HandleEvent(CXPEvent & {...}) line 903 + 27 bytes
CSceneComponent::HandleEvent(CXPEvent & {...}) line 2901
CPluginInstance::PlatformHandleEvent(void * 0x0012fa8c) line 3053 + 35 bytes
CPluginInstance::HelperButtonProc(HWND__ * 0x00070318, unsigned int 514, 
unsigned int 0, long 14614761) line 615
USER32! 77e12e98()
USER32! 77e130e0()
USER32! 77e15824()
nsAppShellService::Run(nsAppShellService * const 0x00ea3440) line 468
main1(int 1, char * * 0x003588c8, nsISupports * 0x00000000) line 1304 + 32 bytes
main(int 1, char * * 0x003588c8) line 1632 + 37 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 77e97d08()

Comment 5

[Peter Lubczynski]

This "context" problem might be related to the stream one in bug 116232.

Comment 6

[Michael Dunn]

Please see:
http://bugscape.mcom.com/show_bug.cgi?id=11842

Comment 7

[Marek Z. Jeziorek]

are there any workarounds or this needs to be fixed?

Comment 8

[av (gone)]

John, looks like your knowledge might help here. Could you please take a look? 
Bounce it back to me if you feel this is not correct.

Comment 9

[Patrick C. Beard]

The stack crawl seems to indicate that the plugin is processing a windows event,
thus there may not be anything pushed on the global JSContext stack. Would this
explain why relative URLs aren't working?

Comment 10

[John Bandhauer]

I think what Patrick said is the key. When there is no JSContext on the per
thread JSContext stack xpconnect uses the 'safe context' for the current thread
in order to call JS code. For the main thread the safe context is the hidden
window's JSContext. So, any relative URL built under those conditions would try
to be relative to the hidden window. lxr shows that the hidden window's url is
'about:blank'. I suppose that is why this stack ends up in
nsAboutProtocolHandler::NewURI. The assert there seems reasonable to me. 

Should the plugin system wrap the call into the native plugin with a push/pop of
the current window's JSContext? IT sems to me that the fix is either that or
make the plugin do its own workaround.

Comment 11

[Peter Lubczynski]

From the stack, it doesn't look like we are going through plugin code.

Would it work and be acceptable to use NPN_GetURL for window.open calls?

Comment 12

[Ari Berger]

Nope.  We used to use GetURL.  We changed it because GetURL runs the javascript 
asynchronously, and very low priority at that.  When we are doing any 
significant amount of rendering, the javascript just won't get executed.  
Patrick helped me set up this method of using XPConnect to eval() the 
javascript because that is synchronous.

Comment 13

[Ari Berger]

Let me clarify how I am executing the javascript- maybe there could be a fix 
somewhere here.
I have an xpconnect property named evaluator.  I execute the following 
javascript though GetURL:
{
var vmpEvaluator = { 	Evaluate: function(message) { eval(message); } }; 
for (var i = 0 ; i<document.embeds.length;i++) 
  {
   try 
	{
        	document.embeds[i].evaluator =vmpEvaluator;
	}
  catch (err)
	{
	}
 }
}
When I want to execute javascript, say "window.open('popup.html')", I set the 
property Evaluator to the javascript I want to execute.  Because anything you 
put in the Evaluator gets sent to eval(), you then evaluate that js, apparently 
with no context.  
Am I explaining this clearly?

Comment 14

[Michael Dunn]

Please update this for me. Is there a Viewpoint side workaround or do we need to
make a code change?

Comment 15

[Ari Berger]

There is a Viewpoint side workaround in content, but it is very undesirable and 
content creators may refuse to use it.  That is why this bug is very serious 
but not a blocker for us. The workaround is to use full (absolute) URLs in the 
content instead of relative URLs.  This is generally bad practice and content 
designers do not want to do this because it locks their content to a specific 
directory on a specific server.

That is why we really would like a fix.  As I said, very serious, but not a 
blocker.
I am also open to other suggested workarounds.

Comment 16

[Peter Lubczynski]

We need to push the right javascript context on the stack somewhere. Here are
some possible ideas:

1. Somehow go through plugin code where we can do this. But I don't see any
plugin code on the stack where we could make this change. Maybe there is a way
the plugin can get the right JS context and push it on the stack? Any ideas?

2. Patrick suggested running the window.open event off our timer (like through
setTimeout) instead of a Windows timer because the js context should then be on
the stack.

Comment 17

[Patrick C. Beard]

> ------- Additional Comments From [e-mail] 2002-01-24 11:32 -------
> There is a Viewpoint side workaround in content, but it is very undesirable and 
> content creators may refuse to use it.  That is why this bug is very serious 
> but not a blocker for us. The workaround is to use full (absolute) URLs in the 
> content instead of relative URLs.  This is generally bad practice and content 
> designers do not want to do this because it locks their content to a specific 
> directory on a specific server.

One possibility would be to use script to compute the absolute URLs
using the document's URL. I agree that this is desirable to fix. I have
an idea about wrapping your XPConnect wrapped object in an object that
ensures the proper JSContext is pushed.

Comment 18

[Michael Dunn]

What is the next step with this bug? ETA?

Comment 19

[Michael Dunn]

Moving to Patrick

Comment 20

[Patrick C. Beard]

Attachment: Trunk patch to implement JS context stack pushing.

This adds a new variable to pass to NPN_SetValue, NPPVjavascriptPushCallerBool,
which if true, pushes the JSContext associated with the document this plugin
instance is part of onto the global nsIJSContextStack, and if false, pops the
current JSContext from the stack.

Comment 21

[Patrick C. Beard]

So, to reiterate, which this patch in place, then when you want to call the 
XPConnect-wrapped JS object from your plugin, you'd wrap the call in 
code like follows:

if (NPN_SetValue(pluginInstance,
                                 NPPVjavascriptPushCallerBool,
                                 (void*)TRUE) == NPERR_NO_ERROR) {
   myJSObject->Eval("1 + 1");
   SetValue(pluginInstance,
                     NPPVjavascriptPushCallerBool,
                     (void*)FALSE);
}

Please try the patch on the branch.

Comment 22

[Patrick C. Beard]

Attachment: JS context stack patch

Slight change for correct error handling.

Comment 23

[Patrick C. Beard]

Attachment: JS context stack patch #3

Remove garbage from end of patch.

Comment 24

[Patrick C. Beard]

Attachment: JS context stack patch #4

Remove garbage for real (don't use IE to attach patches)!

Comment 25

[Peter Lubczynski]

Attachment: patch #4 applied to the 0.9.4 branch

Here's the same patch as above, but I've modified it to apply to the 0.9.4
branch.

Comment 26

[Ari Berger]

I tried patch #4 applied to the branch and it worked perfectly.
Thanks!

Comment 27

[Michael Dunn]

Plusing for 094 checkin

Comment 28

[Michael Dunn]

Sorry a little overzealous. Can we have a r and sr? Will plus after review.

Comment 29

[Peter Lubczynski]

r=peterl on beard's approach with one question:

What happens if the plugin failes to pop the JS context off the stack?

Comment 30

[Johnny Stenback (:jst)]

Comment on attachment 67004 [details] [diff] [review]
JS context stack patch #4

sr=jst

Comment 31

[John Bandhauer]

beard: Did you decide that we do not need additional refcounting to ensure that
the window's nsJSContext (and thus the JSContext) stays alive until the pop
occurs? I'm iffy unless you say that you determined it is for sure not
necessary. Or is this another thing to document that the plugin needs to be
careful about? 

Also, It should be documented that using this feature on any thread other than
the main thread will likely cause very bad things to happen.

No big comments in the patch to any 'end-develper' headers explaining all this?

peterl: Not popping is just yet another one of the many ways that a plugin can
crash the process. Eventually the window will close, the JSContext will die, and
someone will try to use it and crash. This is a sharp tool and must be used
correctly.

Comment 32

[Patrick C. Beard]

Fixes checked in on both trunk and branch.

Comment 33

[shrirang khanzode]

with the 0130 branch build (0.9.4), clicking on the sphere stil does not open 
the popup for me. Am going to try this again on the next spin that comes out. 
Maybe the fix did not make it in this build ( which I doubt tho)...but, anywho, 
wanted to let everyone know this. 

Comment 34

[Patrick C. Beard]

shrir, you'll need an updated build of the viewpoint plugin as well.

Comment 35

[shrirang khanzode]

yeah,I got the latest that was available on the link mentioned in this 
bug...but am afraid it's not yet updated for this fix. Thx!!

Comment 36

[Ari Berger]

Sorry- I'll put it up this evening and let you know when it is there.

Comment 37

[Ari Berger]

Apologies.  I forgot to note that I put up a new build.

Comment 38

[shrirang khanzode]

yes, testcase works as desired now. verified on 0.9.4 branch.

Comment 39

[shrirang khanzode]

v