Closed Bug 1131707 Opened 9 years ago Closed 9 years ago

XPConnect Function forwarders don't properly forward construction

Categories

(Core :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: fdsc, Assigned: bholley)

References

Details

Attachments

(3 files, 2 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 5.1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/27.0.1453.93 Safari/537.36

Steps to reproduce:

My extension "Http UserAgent Cleaner" have function for substitution time zone.

Example

var httpuacleaner_getTimezoneOffset = Cu.exportFunction(function()
				{
					return -valTZ;
				}, window.wrappedJSObject);

Object.defineProperty(window.wrappedJSObject.Date.prototype,	"getTimezoneOffset", 	{value: httpuacleaner_getTimezoneOffset, enumerable : true, configurable : true});

---------

However, when I try the whole Date constructor with their function, the Date has been exported without prototype. I did not find help on how to export the prototype to create the object.

However, this means the inability to create a full object with the possibility of export.



Actual results:

Exported Date.prototype is undefinded. Date instanceof Function is true.


Expected results:

Date.prototype must be exported
Product: Firefox → Core
OS: Windows XP → Windows 7
Hardware: x86 → x86_64
Depends on: 861219
OS: Windows 7 → All
Hardware: x86_64 → All
Summary: Components.utils.exportFunction does not export prototype to window.wrappedJSObject → Components.utils.exportFunction does not export new date.prototype to window.wrappedJSObject
(In reply to fdsc from comment #0)
> User Agent: Mozilla/5.0 (Windows NT 5.1) AppleWebKit/537.36 (KHTML, like
> Gecko) Chrome/27.0.1453.93 Safari/537.36
> 
> Steps to reproduce:
> 
> My extension "Http UserAgent Cleaner" have function for substitution time
> zone.
> 
> Example
> 
> var httpuacleaner_getTimezoneOffset = Cu.exportFunction(function()
> 				{
> 					return -valTZ;
> 				}, window.wrappedJSObject);
> 
> Object.defineProperty(window.wrappedJSObject.Date.prototype,
> "getTimezoneOffset", 	{value: httpuacleaner_getTimezoneOffset, enumerable :
> true, configurable : true});
> 
> ---------
> 
> However, when I try the whole Date constructor with their function, the Date
> has been exported without prototype. I did not find help on how to export
> the prototype to create the object.

Can you clarify what you mean by this? Maybe one of:
|new Date();|
|new window.Data();|
|new window.wrappedJSObject.Date()|
|window.eval('new Date()')|

See https://developer.mozilla.org/en-US/docs/Xray_vision
Hello

Install extension from attachement, load page with JavaScript enabled.

In console FireBug enter commands and see

// For Object.defineProperty(window.wrappedJSObject.Date.prototype,"toString"
// if in the extension make with uncomment code

> new Date()
Date {This is DatePTest Date Object.toString}

> Date()
"Mon May 04 2015 23:51:57 GMT+0300"

Date().toString()
"Mon May 04 2015 23:52:36 GMT+0300"




// For Object.defineProperty(window.wrappedJSObject,	"Date"
// In extension now

// It's work. Time has been changed

> Date()
Date {Sat May 02 2015 02:00:20 GMT+0300}

// It's not work
> new Date()
TypeError: Date is not a constructor

> Date.prototype
undefined


---------------------------------------------------------------------------

Code:

var DateObject = Cu.exportFunction(function(val)
	{
		console.error('DateObject called');
		if (val)
		{
			console.error('with walue ' + value);
			return new window.Date(val - 1000*3600*7*10);
		}
		else
		{
			console.error('without walue');
			return new window.Date(new window.Date().getTime() - 1000*3600*7*10);
		}
	}, window.wrappedJSObject);

var DateObjectClassStr = Cu.exportFunction(function()
	{
		return 'This is DatePTest Date Class.toString';
	}, window.wrappedJSObject);

	DateObject.prototype = Cu.createObjectIn(window.wrappedJSObject);
	DateObject.prototype.toString = DateObjectClassStr;

Object.defineProperty(window.wrappedJSObject,	"Date", 	{value: DateObject, enumerable : true, configurable : true});


---------------------------------------------------------------------------

In DateObject constructor function returned object.
E.g.

var F = function() {return {a: 'b'};};

F() returned
Object { a="b"}

new F() returned
Object { a="b"}

But Date() returned Date
new Date() returned error

Similary new window.Date() and eval('new Date()')
(In reply to fdsc from comment #2)
> 	DateObject.prototype = Cu.createObjectIn(window.wrappedJSObject);

This isn't going to work - DateObject itself is just a wrapper around the underlying closure that you pass to exportFunction. DateObject.prototype will not automatically be the prototype of the objects that your anonymous function creates.


(In reply to fdsc from comment #2)
> var customProto = Cu.cloneInto({ toString: DateObjectClassStr }, window.wrappedJSObject, { cloneFunctions: true });
> var DateObject = Cu.exportFunction(function(val)
> 	{
> 		console.error('DateObject called');
>               var d;
> 		if (val)
> 		{
> 			console.error('with walue ' + value);
> 			d = new window.Date(val - 1000*3600*7*10);
> 		}
> 		else
> 		{
> 			console.error('without walue');
> 			d = window.Date(new window.Date().getTime() - 1000*3600*7*10);
> 		}
>               Object.setPrototypeOf(d.wrappedJSObject, customProto);
> 	}, window.wrappedJSObject);

But do you really want to be creating an entirely custom proto? It seems like you should just patch over 'toString' on the existing proto.
> do you really want to be creating an entirely custom proto?

I already have the toString function and many other functions-substitutes in prototype. They are in https://addons.mozilla.org/ru/firefox/addon/http-useragent-cleaner/ now (in developer channel). If want, see lib/main.js file with keyword window.wrappedJSObject.Date.prototype


But I need to control precisely the object constructor and Date() function calls.
(In reply to fdsc from comment #4)
> But I need to control precisely the object constructor and Date() function
> calls.

Right - so just override the date constructor like you're doing - no need to mess with the prototype. |new window.Date(...)| will create an object whose prototype is window.Date.prototype (though you'll need to waive Xrays to see the custom modifications from chrome).
I need to override three things:
1. new Date()
2. Date()
3. new Date().functions calls

It is not only the constructor
(In reply to fdsc from comment #6)
> I need to override three things:
> 1. new Date()
> 2. Date()

These two can be fixed by overriding the constructor...

> 3. new Date().functions calls

...and this by monkey-patching window.Date.prototype.wrappedJSObject, right? Or am I missing something?
Attached file workaround not work (obsolete) —
It doesn't work.

I think that the old prototype is overwritten with the overwrite new constructor. It would be strange if it worked, and it really doesn't work.
(In reply to fdsc from comment #8)
> Created attachment 8601178 [details]
> workaround not work

Can you please post a reduced code snippet rather than attaching the entire add-on? The issue we're discussing here is a very specific one, and most situations can be demonstrated in a couple of lines of independent sample code.
I just did what you said. If you want to see the code, I have attached an illustration in extension.

If you trust me, cause code is not necessary. It trivial. If not, you should see a working version, as excerpts of code can be wrong.

In the attached addition there is nothing interesting, except that I already wrote. Your suggestion does not work. I have attached in case you want to look at working code.
I'd like to help you, but I don't have time to look through the entire extension. If you want to continue here, please post a small self-contained snippet that I can execute in scratchpad which demonstrates the problem you're running into.
I told you that your solution does not work. I don't understand what code you want.
If you are too lazy to unzip the file, I will do it for you.


const {Cc, Ci, Cu, Cr, Cm, components} = require("chrome");
Cu.import("resource://gre/modules/XPCOMUtils.jsm");


var DatePTest = {};

DatePTest.getProtocolForDocument = function(document, withoutDoublePoint)
{
	var protocol = '';
	if (document.location && document.location.protocol)
		protocol = document.location.protocol;
	else
		try
		{
			protocol = /^[^:]+:/i.exec(document.URL)[0];
		}
		catch (e)
		{
			return "";
		}
	
	if (withoutDoublePoint)
		return protocol.substring(0, protocol.length - 1);

	return protocol;
};


DatePTest.observer = function(subject)
{
	var document = subject.subject;
	var aTopic   = subject.type;
	var aData    = subject.data;
	
	if (!document || (!document.URL && !document.location)/* || !document.defaultView || !document.defaultView.navigator*/)
		{
			return;
		}

	var protocol = "";
	protocol = DatePTest.getProtocolForDocument(document);

	if (
			   protocol == "resource:"
			|| protocol == "data:"
			|| protocol == "about:"
			|| protocol == "moz-safe-about:"
			|| protocol == "moz-filedata:"
			|| protocol == "moz-icon:"
			|| protocol == "mediasource:"
			|| protocol == "chrome:"
			|| protocol == "blob:"
			|| protocol == "file:"
			|| protocol == "view-source:"
		)
	{
		// console.warn("HTTP UA Cleaner ignored document " + document.URL/*document.location.href*/);
		return;
	}
	
	var window = document.defaultView;
	
	if (!window)
		return;
	
	var DateObject = Cu.exportFunction(function(val)
	{
		console.error('DateObject called');
		if (val)
		{
			console.error('with walue ' + value);
			return new window.Date(val - 1000*3600*7*10);
		}
		else
		{
			console.error('without walue');
			return new window.Date(new window.Date().getTime() - 1000*3600*7*10);
		}
	}, window.wrappedJSObject);
	/*
	var GetDateObject = Cu.exportFunction(function(val)
	{
		return DateObject;
	}, window.wrappedJSObject);
*/
	var DateObjectStr = Cu.exportFunction(function()
	{
		return 'This is DatePTest Date Object.toString';
	}, window.wrappedJSObject);
	
	var DateObjectClassStr = Cu.exportFunction(function()
	{
		return 'This is DatePTest Date Class.toString';
	}, window.wrappedJSObject);
/*
	DateObject.prototype = Cu.createObjectIn(window.wrappedJSObject);
	DateObject.prototype.toString = DateObjectClassStr;
*/
	/*if (1 == 1)
	{*/
		Object.defineProperty(window.wrappedJSObject,	"Date", 	{value: DateObject, enumerable : true, configurable : true});
	/*}
	else
	{*/
		Object.defineProperty(window.Date.prototype.wrappedJSObject,	"toString", 	{value: DateObjectStr, enumerable : true, configurable : true});
	//}
};

exports.main = function (options, callbacks)
{
	var events = require("sdk/system/events");
	events.on("document-element-inserted", 	DatePTest.observer);
};

exports.onUnload = function (reason)
{
};
Attached file workaround not work
Previous implementation was incorrect.

> Date.prototype
Date {This is DatePTest Date Object.toString}

> Date()
Date {This is DatePTest Date Object.toString}

> new Date()
TypeError: Date is not a constructor

Date.prototype succeed to override.
But in prototype have overrided object. And Date not a constructor.

I tried to override it in the following ways

// 1
var a = window.Date.prototype;
... (Date override)
window.wrappedJSObject.Date.prototype = a;


// 2
window.wrappedJSObject.Date = Cu.cloneInto(window.Date.prototype, window.wrappedJSObject);


// 3
var PGet = function()
	{
		console.error('PGet');
		return Cu.cloneInto(window.Date.prototype, window.wrappedJSObject);
	};
	
	var PSet = function()
	{
		console.error('PSet');
	};
	
	Object.defineProperty(window.wrappedJSObject.Date,	"prototype", 	{get: PGet, set: PSet, enumerable : true, configurable : true});


// 4
window.wrappedJSObject.Date.prototype = Cu.cloneInto(window.Date.prototype, window.wrappedJSObject);
Attachment #8601178 - Attachment is obsolete: true
No longer depends on: 861219
Summary: Components.utils.exportFunction does not export new date.prototype to window.wrappedJSObject → Components.utils.exportFunction does not export date.prototype and other func.prototype to window.wrappedJSObject (exported func not constructor)
(In reply to fdsc from comment #12)
> If you are too lazy to unzip the file, I will do it for you.

Hey now - I'm under no obligation to help you out here. If you're hoping to get people to voluntarily take time and offer suggestions, it helps to be more friendly. ;-)

The reason self-contained testcases are useful is that they allow people to focus on the problem at hand and actually _execute_ the sample code in a simple way. You may be very familiar with your extension, but a random person helping you in their spare time is unlikely to want to install your extension, figure out how it works, and reproduce the issue.

I just threw together a testcase of the form I was asking for. You should be able to execute it in a browser scratchpad, and see it work properly.

Because I was able to run the code while hacking it up, I noticed that exported functions don't transparently forward the |construct| bit, which means that they treat |new foo()| and |foo()| identically (this may or may not be the problem you were running into). This is usually fine, but Date has different behavior between the two for legacy reasons. The eval() workaround in the sample code should work fine for your purposes, but I'll file a bug to fix things to be more transparent.

var sb = new Cu.Sandbox(null);
var dateCtor = sb.Date;
var dateProto = sb.Date.prototype;
var wrappedCtor = Cu.exportFunction(function(val) {
  val = val || (new Date()).getTime();
  var d = new dateCtor(val - 1000*3600*7*10);
  console.log(Cu.waiveXrays(d).getTime());
  return d;
}, sb);
Cu.waiveXrays(sb).__wrappedCtor = wrappedCtor;
sb.eval('Date = function(val) { "use strict"; return this ? __wrappedCtor(val) : __wrappedCtor(val).toString();};')
dateProto.wrappedJSObject.toString = Cu.exportFunction(function() { return "custom date string"; }, sb);

((new Date()).getTime()) + ", " +
sb.eval('(new Date()).getTime()') + ", " +
(new Date().toString()) + ", " +
sb.eval('(new Date()).toString()')
(In reply to Bobby Holley (:bholley) from comment #14)
> I'll file a bug to fix things to be more transparent.

Actually I'll just use this bug, if that's alright with you. Please file followups if you run into additional issues.
Summary: Components.utils.exportFunction does not export date.prototype and other func.prototype to window.wrappedJSObject (exported func not constructor) → XPConnect Function forwarders don't properly forward construction
Frankly, I don't understand.


I don't understand how you run the privileged code without starting the extension.

I completely misunderstood that code that you mentioned. Could you explain where that execute.
Your code is too complicated for me. I don't think this code is simpler than the extension.


---------------------------------------------------------------------------------

And, I noticed that the constructor works without exporting. I'm not sure that this is not a bug. It seemed to me that such functions should be prohibited for export.

Object.defineProperty(window.wrappedJSObject, 'Date', 
{value: function(val)
{
    if (this.document || this.Cu)
    {
        return new window.Date().wrappedJSObject.toString();
    }

    if (val)
    {
        return new window.Date(val);
    }
    else
    {
        return new window.Date();
    }
}
, enumerable : true, configurable : true
});

And new Date() and Date() is works. (and eval is too)
(In reply to fdsc from comment #18)
> I don't understand how you run the privileged code without starting the
> extension.

https://developer.mozilla.org/en-US/docs/Tools/Scratchpad#Running_Scratchpad_in_the_browser_context

(Sorry, I seem to remember pasting that link into this bug before, but I guess I didn't).

You can also go to |about:| and paste it into the console.

> And, I noticed that the constructor works without exporting. I'm not sure
> that this is not a bug. It seemed to me that such functions should be
> prohibited for export.

For legacy reasons, we need to allow certain functions to be called. But they don't appear like normal functions (and you can't do .bind etc on them), which is why exportFunction is preferable.
Comment on attachment 8601701 [details] [diff] [review]
Transparently forward the construct bit for function forwarders. v1

Review of attachment 8601701 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/ExportHelpers.cpp
@@ +328,2 @@
>      if (!thisObj) {
>          return false;

I don't get it. Seems like this will return false always in the args.isConstructing case now... which I don't think is the goal here. (also the {} seems to be unnecessary)

@@ +346,5 @@
>  
>          RootedValue fval(cx, ObjectValue(*unwrappedFun));
> +        if (args.isConstructing()) {
> +            if (!JS::Construct(cx, fval, args, args.rval())) {
> +                return false;

Nit: extra {}
Attachment #8601701 - Flags: review?(gkrizsanits) → review-
> sb.eval('Date = function(val) { "use strict"; return this ? __wrappedCtor(val) : __wrappedCtor(val).toString();};')

It's all wonderful. But what I need to replace sb.eval in the extension? Am I have the eval command inside the window in extension privileged code?
(In reply to fdsc from comment #21)
> It's all wonderful. But what I need to replace sb.eval in the extension? Am
> I have the eval command inside the window in extension privileged code?

You can do window.eval over Xrays, yes.

Be careful though that replacing the Date constructor on arbitrary webpages may cause problems if those webpages are also doing funny things with Date.

Also, in the above code |new Date() instanceof Date| will return false, which will probably break things. You should be able to fix this by doing |wrappedCtor.wrappedJSObject.prototype = dateProto|. You should test it to make sure it works.
Depends on: 1162187
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #20)
> I don't get it. Seems like this will return false always in the
> args.isConstructing case now... which I don't think is the goal here. (also
> the {} seems to be unnecessary)

Hah, good catch! Yeah, that pre-existing code seems pretty bogus, and it was actually preventing the failures in the test from being caught - see bug 1162187.
Attachment #8601701 - Attachment is obsolete: true
Attachment #8602261 - Flags: review?(gkrizsanits)
Comment on attachment 8602261 [details] [diff] [review]
Transparently forward the construct bit for function forwarders. v2

Review of attachment 8602261 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Bobby Holley (:bholley) from comment #23)
> actually preventing the failures in the test from being caught - see bug
> 1162187.

Was wondering how did the test pass... Fun times... :)
Attachment #8602261 - Flags: review?(gkrizsanits) → review+
https://hg.mozilla.org/mozilla-central/rev/3e4fa121bc55
Assignee: nobody → bobbyholley
Status: UNCONFIRMED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Moving from Core::Untriaged to Core::General https://bugzilla.mozilla.org/show_bug.cgi?id=1407598
Component: Untriaged → General
You need to log in before you can comment on or make changes to this bug.