Hello michaeln, nigeltao,
I'd like you to do a code review. Please execute
g4 diff -c 9284351
or point your web browser to
http://mondrian/9284351
to review the following code:
Change 9284351 by [EMAIL PROTECTED] on 2008/12/05 16:09:51 *pending*
A second attempt at solving the "call by reference" crash on FF2 and
FF3.
R=michaeln,nigeltao
[EMAIL PROTECTED]
DELTA=50 (47 added, 0 deleted, 3 changed)
OCL=9284351
Affected files ...
...
//depot/googleclient/gears/opensource/gears/base/common/js_runner_ff_marshaling.cc#13
edit
...
//depot/googleclient/gears/opensource/gears/base/common/js_runner_ff_marshaling.h#11
edit
50 delta lines: 47 added, 0 deleted, 3 changed
Also consider running:
g4 lint -c 9284351
which verifies that the changelist doesn't introduce new style violations.
If you can't do the review, please let me know as soon as possible. During
your review, please ensure that all new code has corresponding unit tests and
that existing unit tests are updated appropriately. Visit
http://www/eng/code_review.html for more information.
This is a semiautomated message from "g4 mail". Complaints or suggestions?
Mail [EMAIL PROTECTED]
Change 9284351 by [EMAIL PROTECTED] on 2008/12/05 16:09:51 *pending*
A second attempt at solving the "call by reference" crash on FF2 and
FF3.
Affected files ...
...
//depot/googleclient/gears/opensource/gears/base/common/js_runner_ff_marshaling.cc#13
edit
...
//depot/googleclient/gears/opensource/gears/base/common/js_runner_ff_marshaling.h#11
edit
====
//depot/googleclient/gears/opensource/gears/base/common/js_runner_ff_marshaling.cc#13
-
c:\GearsWinMo/googleclient/gears/opensource/gears/base/common/js_runner_ff_marshaling.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/base/common/js_runner_ff_marshaling.cc
2008-12-05 16:19:44.000000000 +0000
+++ googleclient/gears/opensource/gears/base/common/js_runner_ff_marshaling.cc
2008-12-05 16:18:55.000000000 +0000
@@ -108,6 +108,9 @@
// the JSClass might be destroyed before all its INSTANCEs are. We choose to
// hold the JSClass objects in a ref-counted SharedJsClasses object, which will
// delete the JSClass'es when there are no longer any references to them.
+
+std::set<std::string> JsContextWrapper::gears_module_names_;
+Mutex JsContextWrapper::gears_module_names_mutex_;
enum JsWrapperDataType {
PROTO_JSOBJECT,
@@ -286,6 +289,9 @@
proto_wrappers_.push_back(proto_data.get());
shared_js_classes_->Insert(alloc_js_class.release());
JS_SetPrivate(cx_, proto, proto_data.release());
+
+ // Add the module name to the global list of Gears module names.
+ InsertGearsModuleName(module_name.c_str());
}
JS_BeginRequest(cx_);
@@ -530,9 +536,23 @@
assert(function_data);
assert(function_data->header.type == FUNCTION_JSOBJECT);
- JsWrapperDataForInstance *instance_data =
- static_cast<JsWrapperDataForInstance*>(JS_GetPrivate(cx, instance_obj));
- if (!instance_data) {
+ JSClass *class_data = JS_GetClass(cx, instance_obj);
+ assert(class_data);
+ // See if this is a Gears class. Note that IsGearsModuleName doesn't need
+ // to test against the list of all possible Gears module names. It suffices
to
+ // test against the list of module names that were created so far (or else
+ // we would get a call to a method on a module that was never instantiated
+ // before, which is impossible).
+ // A previous attempt to implement this test was as follows:
+ // JsWrapperDataForInstance *instance_data =
+ // static_cast<JsWrapperDataForInstance*>(JS_GetPrivate(cx,
instance_obj));
+ // if (!instance_data) { (...) }
+ // This relied on the fact that instance_data must have been previously set
+ // by Gears or must be NULL otherwise. While this worked on FF3, it did not
+ // work on FF2 since on this browser, at JSObject creation time, the
+ // JSObject's "fslots" array is left unitialized so JS_GetPrivate() may
return
+ // a completely bogus value.
+ if (!IsGearsModuleName(class_data->name)) {
// We can get here in the case of a Gears method being invoked via
// a reference, as in the example below:
// someModule = google.gears.factory.create('beta.someModule');
@@ -550,6 +570,10 @@
STRING16(L"Member function called without a Gears object."));
return JS_FALSE;
}
+
+ JsWrapperDataForInstance *instance_data =
+ static_cast<JsWrapperDataForInstance*>(JS_GetPrivate(cx, instance_obj));
+ assert(instance_data);
assert(instance_data->header.type == INSTANCE_JSOBJECT);
assert(instance_data->module);
assert(function_data->dispatch_id);
@@ -587,3 +611,17 @@
return !call_context.is_exception_set() ? JS_TRUE : JS_FALSE;
}
+
+// static
+bool JsContextWrapper::IsGearsModuleName(const char* module_name) {
+ assert(module_name);
+ MutexLock locker(&gears_module_names_mutex_);
+ return gears_module_names_.find(module_name) != gears_module_names_.end();
+}
+
+// static
+void JsContextWrapper::InsertGearsModuleName(const char* module_name) {
+ assert(module_name);
+ MutexLock locker(&gears_module_names_mutex_);
+ gears_module_names_.insert(module_name);
+}
\ No newline at end of file
====
//depot/googleclient/gears/opensource/gears/base/common/js_runner_ff_marshaling.h#11
-
c:\GearsWinMo/googleclient/gears/opensource/gears/base/common/js_runner_ff_marshaling.h
====
# action=edit type=text
--- googleclient/gears/opensource/gears/base/common/js_runner_ff_marshaling.h
2008-12-05 16:19:44.000000000 +0000
+++ googleclient/gears/opensource/gears/base/common/js_runner_ff_marshaling.h
2008-12-05 15:47:57.000000000 +0000
@@ -35,6 +35,7 @@
#include <gecko_internal/jsapi.h>
#include "gears/base/common/base_class.h"
+#include "gears/base/common/mutex.h"
#include "gears/base/common/scoped_refptr.h"
#include "third_party/scoped_ptr/scoped_ptr.h"
@@ -118,6 +119,9 @@
static JSBool JsWrapperCaller(JSContext *cx, JSObject *obj,
uintN argc, jsval *argv, jsval *retval);
+ static bool IsGearsModuleName(const char* module_name);
+ static void InsertGearsModuleName(const char* module_name);
+
JSContext *cx_;
JSObject *global_obj_;
@@ -134,6 +138,11 @@
scoped_refptr<SharedJsClasses> shared_js_classes_;
bool cleanup_roots_has_been_called_;
+ // Global set of Gears module names. The set gets populated
+ // automatically as Gears modules are created.
+ static std::set<std::string> gears_module_names_;
+ static Mutex gears_module_names_mutex_;
+
DISALLOW_EVIL_CONSTRUCTORS(JsContextWrapper);
};
