Closed Bug 560751 Opened 14 years ago Closed 14 years ago

[jsd] function objects sometimes have unexpected wrappers

Categories

(Core :: JavaScript Engine, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED INVALID
Tracking Status
blocking2.0 --- -
status2.0 --- wanted
blocking1.9.2 --- -
status1.9.2 --- wanted
status1.9.1 --- wanted

People

(Reporter: johnjbarton, Assigned: Waldo)

References

Details

(Keywords: regression, Whiteboard: [firebug-p1])

This is a follow up to But 521010
when I issue
 found = this.jsd.wrapValue(fn).script;
for fn in a Web page declared like:
function onExecuteTest()
{
 console.log("Hello World!");
} 
then script is null. (It does work for extension functions in browser.xul)

In the failing case we exit with NULL:
JSDScript*
jsd_GetScriptForValue(JSDContext* jsdc, JSDValue* jsdval)
{
    JSContext* cx = jsdc->dumbContext;
    jsval val = jsdval->val;
    JSFunction* fun;
    JSExceptionState* exceptionState;
    JSScript* script = NULL;
    JSDScript* jsdscript;

    if (!jsd_IsValueFunction(jsdc, jsdval))
        return NULL;
---------------^ HERE

Note that the js code checks the argument:
if (typeof(fn) == "function" || fn instanceof Function)
and the result is true.

I poked around and discovered that this.jsd.wrapValue(fn).jsParent looks like
our function and it has the correct 'script' property. I don't know what the
jsParent is but it gives the right result.

Then Boris says:

Your issue is that "fn" is a security wrapper around a content function, so not
actually a scripted function (and hence doesn't have a script).  jsParent is
... not very well defined, but for security wrappers around functions the
parent is the original function as a hack to make it possible to easily find
the original function when the wrapper is called.

Can you please file a followup bug on this case and cc mrbkap and me on it? 
It's possible that jsd should auto-unwrap here, if it doesn't open security
holes.
1. Install Firebug 1.6a9, http://getfirebug.com/releases/firebug/1.6X/
2. open https://getfirebug.com/tests/content/commandLine/debug.html
3. reload
4. Set a breakpoint in jsd_val.c, in jsd_GetScriptForValue
5. follow the web page instructions

You will hit once and fail as above.

Firebug 1.6a10 will hit twice, as the second one is the jsParent hack above.
Whiteboard: [firebug-p3]
Blocks: 521010
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
blocking2.0: --- → ?
Keywords: regression
We'd take a patch once baked, not sure it blocks the security and stability releases. Is it a common crasher for Firebug? You marked as a P3, so I figured not. Renominate if that was wrong of me.
blocking1.9.1: ? → ---
blocking1.9.2: ? → -
There is no crash here. The jsParent hack allows Firebug to work in all the cases we have so far. So this is a cleanliness and grooviness bug, not a security or stability bug ;-)
This bug now breaks Firebug on FF 3.7, we guess because __parent__ changes described here:
http://whereswalden.com/2010/05/07/spidermonkey-change-du-jour-the-special-__parent__-property-has-been-removed/

I asked on the blog post about the relation between __parent__ and jsdIValue.jsParent, but I did not understand the answer. 

In addition to this bug, jsParent is essential for Chromebug to work.
Whiteboard: [firebug-p3] → [firebug-p1]
> but I did not understand the answer. 

The answer is that jsdIValue.jsParent is non-null anytime __parent__ is nonnull (but there are cases when __parent__ is null but .jsParent is still nonnull).  And the other part of the answer is that .jsParent should not have stopped working.
> So this is a cleanliness and grooviness bug, not a security or stability bug ;-)

So that makes it something we're not going to block on, but wanted, sure.
blocking2.0: ? → -
status2.0: --- → wanted
Whiteboard: [firebug-p1] → [firebug-p3]
> So that makes it something we're not going to block on, but wanted, sure.

Except that comment was based on using __parent__ in the Firebug code.  And then we removed __parent__ ('cause it was unclean and all).  So their code really is broken now, as I understand it.  Renominating, but John, please tell me if I misunderstood the situation.
blocking2.0: - → ?
In the interest of avoiding more confusion, I think we should leave this bug on my original test case, the bug being that we should not have gotten the wrapper.

I'll file a new bug for the hackaround involving jsParent and it should only be an issue on trunk. (sorry should have done that at comment 4, it's just that I would need the hackaround if this bug was easy to fix).

So this bug does not block, thanks!
Ok so now this bug does break Firebug. In FF 4.0b4, as Boris says in comment 7, we no longer hack around the regression and our test case fails.
Whiteboard: [firebug-p3] → [firebug-p1]
Version: unspecified → Trunk
Just changing the platform flag to All (I can also reproduce that on Windows).

Are all flags properly set here?
(to get enough attention)

Honza
OS: Linux → All
blocking2.0: ? → beta6+
waldo: i'm going on vacation for a month. i believe in "you break it, you bought it".

Your blog post more or less indicates you broke it.
Assignee: nobody → jwalden+bmo
blocking2.0: beta7+ → beta8+
This bug confuses me.  It's saying something about the jsParent property being broken due to wrapping, and then somehow in comment 4 the old __parent__ property is brought into the bug, and I have no idea what the connection is supposed to be between the two.  My changes should not have affected jsParent in any way, as best as I can remember, so I'm confused as to why there's a claim that I broke this, and therefore I'm the person to fix it.  It sounds much more like a should-we-be-unwrapping security decision that mrbkap/bz are much more qualified to make than I would be, with no connection to __parent__ at all.
I can't tell where we are here now; John has morphed this bug several times.  The .jsParent thing should work, but then he said it doesn't....  Would welcome clarification on that from him!
My original report remains accurate, and based on that I believe that the test case in comment #1 remains valid.

As I said in comment #3, something is broken, but I have a way to hack around it in FF 3.6.

This hack does not work in FF 4.0. Here's where we lose our way. The only solid fact we have is that back when 4.0 was 3.7, my hack failed. The rest of the comments are about the possible relationship between two changes with 6 characters in common:
    1. my 3.6 .jsParent hack fails in 4.0
and 2. __parent__ was removed between 3.6 and 4.0.
I have no information about this connection beyond those 6 characters.

This bug should really be about my original report: Boris' "It's possible that jsd should auto-unwrap here, if it doesn't open security
holes."

If we decide the current 3.6 result is correct and my hack is actually the right operation, then we should morph this bug into one about jsParent on FF4. But only after jsd comes out of intensive care after from its run-in with the JaegerMonkey.

Hope this helps...
John, noting involving .jsParent should have broken.  If it did, something is very wrong and we need to find and fix that something.  Please file a separate bug with some information on what you're doing with jsParent and how the behavior changed?  And a regression range, if possible.
blocking2.0: beta8+ → beta9+
(In reply to comment #15)
> John, noting involving .jsParent should have broken.  If it did, something is
> very wrong and we need to find and fix that something.  Please file a separate
> bug with some information on what you're doing with jsParent and how the
> behavior changed?  And a regression range, if possible.

Renominate if these questions get well answered.
blocking2.0: beta9+ → -
I seem to have managed to get this bug (the extra wrapper) focused on the wrong topic (.jsParent)

Let's just start this one all over once we get FF4.0 far enough along to be confident that the test case in comment 2 is meaningful.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → INVALID
Blocks: 624316
Component: JavaScript Debugging/Profiling APIs → JavaScript Engine
You need to log in before you can comment on or make changes to this bug.