Last Comment Bug 287107 - xpcwrappedjsclass.cpp rev 1.73 regressed "return" of NS_COMFALSE (Components.returnCode)
: xpcwrappedjsclass.cpp rev 1.73 regressed "return" of NS_COMFALSE (Components....
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla2.0
Assigned To: Mark Hammond [:markh]
:
:
Mentors:
: 288698 (view as bug list)
Depends on:
Blocks: 194043 287490 287846 288698 296430 474121 474122 474124 474125
  Show dependency treegraph
 
Reported: 2005-03-21 12:51 PST by Brendan Eich [:brendan]
Modified: 2014-12-16 17:16 PST (History)
29 users (show)
benjamin: wanted1.9.1-
pavlov: blocking1.9-
reed: wanted1.9+
bugzillamozillaorg_serge_20140323: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
proposed fix (4.41 KB, patch)
2005-03-21 13:13 PST, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
fix, v2 (5.87 KB, patch)
2005-03-21 17:07 PST, Brendan Eich [:brendan]
dbradley: review+
jst: superreview+
Details | Diff | Splinter Review
Minimized patch to fix xpconnect, and break a bunch of bad clients (2.01 KB, patch)
2005-03-29 14:21 PST, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
testcases (624 bytes, text/plain)
2007-03-11 04:29 PDT, timeless
no flags Details
patch for thought (2.24 KB, patch)
2007-03-11 04:30 PDT, timeless
no flags Details | Diff | Splinter Review
0001-Bug-287107-make-Components.returnCode-be-the-xpcom-n.patch (16.94 KB, patch)
2014-11-26 22:00 PST, Mark Hammond [:markh]
bobbyholley: review+
Details | Diff | Splinter Review
0001-Bug-287107-make-Components.returnCode-be-the-xpcom-n.patch (16.41 KB, patch)
2014-11-27 17:21 PST, Mark Hammond [:markh]
markh: review+
Details | Diff | Splinter Review
0002-handle-nested-invocations.patch (7.18 KB, patch)
2014-11-27 17:38 PST, Mark Hammond [:markh]
bobbyholley: review-
Details | Diff | Splinter Review
0002-handle-nested-invocations.patch (8.53 KB, patch)
2014-12-07 19:56 PST, Mark Hammond [:markh]
bobbyholley: review+
Details | Diff | Splinter Review
0001-Bug-287107-make-Components.returnCode-be-the-xpcom-n.patch (21.71 KB, patch)
2014-12-08 18:57 PST, Mark Hammond [:markh]
markh: review+
Details | Diff | Splinter Review

Description Brendan Eich [:brendan] 2005-03-21 12:51:00 PST
The problem is visible clearly enough in the diff at
http://tinderbox.mozilla.org/bonsai/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/js/src/xpconnect/src&command=DIFF_FRAMESET&file=xpcwrappedjsclass.cpp&rev1=1.72&rev2=1.73&root=/cvsroot
-- pending_result in nsXPCWrappedJSClass::CallMethod is no longer set early and
unconditionally, as it was in 1.72.

Patch in a minute or two.

Whoever knows a good way to reach t3rmin4t0r (IRC nick, he ran into this when
trying to implement (not script) nsIEnumerator), please let him know about this bug.

dbradley, bc: we need a regression test that gets run here.  What is the state
of xpconnect testing?  jband may have written such a test, as I believe this
case worked.

/be
Comment 1 Brendan Eich [:brendan] 2005-03-21 13:13:45 PST
Created attachment 178145 [details] [diff] [review]
proposed fix
Comment 2 Brendan Eich [:brendan] 2005-03-21 13:16:13 PST
I wrote:
"... pending_result in nsXPCWrappedJSClass::CallMethod is no longer set early and
unconditionally, as it was in 1.72."

I meant "no longer set early *after the getter, setter, or function call* ...."
 Of course pending_result is initialized, but that's before the JS invocation.

/be
Comment 3 Brendan Eich [:brendan] 2005-03-21 17:04:08 PST
Comment on attachment 178145 [details] [diff] [review]
proposed fix

Oops, missed the IDispatch stuff that's configured off by default.

/be
Comment 4 Brendan Eich [:brendan] 2005-03-21 17:07:13 PST
Created attachment 178179 [details] [diff] [review]
fix, v2
Comment 5 Johnny Stenback (:jst, jst@mozilla.com) 2005-03-21 20:26:44 PST
Comment on attachment 178179 [details] [diff] [review]
fix, v2

sr=jst
Comment 6 David Bradley 2005-03-21 21:22:57 PST
One thing that should be check is that pending_result coming from XPCCallContext
is reset with each call. A quick LXR of mPendingResult shows that it's only Set
never initialized. I have a vague memory that it's only set when an error
occurs, and is left uninitialized if there is no exception detected.

I think there may be more here than meets the eye. I can look into this further
later (48 hours) or if someone else wants to run with it that's fine.
Comment 7 Brendan Eich [:brendan] 2005-03-21 23:34:36 PST
dbradley: nsXPCWrappedJSClass::CallMethod calls

    xpcc->SetPendingResult(pending_result);

at line 1006 in the patched file, 1009 in the trunk rev.  I also see this:

$ grep -n !*
grep -n mPendingRes *.cpp
xpccontext.cpp:67:        mPendingResult(NS_OK),
xpccontext.cpp:99:        XPC_LOG_ALWAYS(("mPendingResult of %x", mPendingResult));

So it looks to me as though mPendingResult is initialized, and also set before
each call to NS_OK.  That should be sufficient, although it could be optimized
slightly.

/be
Comment 8 David Bradley 2005-03-22 11:11:34 PST
Comment on attachment 178179 [details] [diff] [review]
fix, v2

r=dbradley

Sorry for the confusion, somehow my LXR search failed to turn up the
initializer in XPCContext
Comment 9 Brendan Eich [:brendan] 2005-03-22 18:40:39 PST
Checked in.  Someone nominate for branches if there's a good reason.

/be
Comment 10 David Bradley 2005-03-24 03:50:22 PST
I had a bad feeling about this. While the pending result value is initialized 
in the constructor I think the problem we're seeing is that the XPCContexts are 
reused across calls. So that as we weave in and out between JS and XPConnect 
we're reusing the same XPCContext.

The fragment in my memory still holds true. The pending result isn't cleared 
and shouldn't be used unless there there was an error. Using it otherwise can 
retrieve previous errors.

We could clear it out before each call. I'm not sure off the top of my head if 
that would have any unintended side effects.
Comment 11 Brendan Eich [:brendan] 2005-03-24 15:40:49 PST
This wasn't the bug that regressed due to the patch in this bug, so it didn't
need to be reopened unless that patch were backed out.  I fixed bug 287490, so
all should be well now.

/be
Comment 12 Myk Melez [:myk] [@mykmelez] 2005-03-26 14:52:08 PST
Note bug 287846, another regression caused by the fix for this bug.
Comment 13 Hermann Schwab 2005-03-28 06:06:05 PST
Shouldn´t this bug be reopened because of patch backed out?

Back out recent attempts to fix 287107 and follow-on bugs; back to drawing board.

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=SeaMonkeyAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-03-28+00%3A25&maxdate=2005-03-28+00%3A25&cvsroot=%2Fcvsroot
Comment 14 Brendan Eich [:brendan] 2005-03-28 19:05:29 PST
Hermann's right, reopening.

/be
Comment 15 Brendan Eich [:brendan] 2005-03-29 14:17:13 PST
Taking.

/be
Comment 16 Brendan Eich [:brendan] 2005-03-29 14:21:31 PST
Created attachment 178981 [details] [diff] [review]
Minimized patch to fix xpconnect, and break a bunch of bad clients

This is for testing.  We need to shake out all the bad JS QI impls that don't
handle a needed IID, or otherwise freak out their callers when their indirectly
thrown NS_NOINTERFACE returnCode propagates, as it will with this fix.

/be
Comment 17 neil@parkwaycc.co.uk 2005-04-07 15:40:18 PDT
(In reply to comment #16)
>We need to shake out all the bad JS QI impls that don't handle a needed
>IID, or otherwise freak out their callers when their indirectly thrown
>NS_NOINTERFACE returnCode propagates, as it will with this fix.
So JS code that thought it was being clever by setting returnCode to
NS_NOINTERFACE was being faked out by an xpconnect null test?
Comment 18 Brendan Eich [:brendan] 2005-04-07 15:58:50 PDT
(In reply to comment #17)
> (In reply to comment #16)
> >We need to shake out all the bad JS QI impls that don't handle a needed
> >IID, or otherwise freak out their callers when their indirectly thrown
> >NS_NOINTERFACE returnCode propagates, as it will with this fix.
> 
> So JS code that thought it was being clever by setting returnCode to
> NS_NOINTERFACE was being faked out by an xpconnect null test?

What null test do you mean?

The problem was that rev 1.73 removed the pending_result reassignment from
xpcc->GetPendingResult(), evacuating it to a new sub-method that's called only
on a failure nsresult.  That breaks all canonical JS-impl'ed QIs -- but callers
don't miss the lack of an error because the canonical JS QI returns null after
setting Components.returnCode = NS_NOINTERFACE.

It also breaks attempts by JS impl's of ugly interfaces such as nsIEnumerator to
result in NS_COMFALSE.  That's what occasioned this bug.

Is some other bug trying to change how JS impl'ed QI should result in
NS_NOINTERFACE?  I hope not.  We have to preserve compatibility, and given what
we specified and supported for a long while, why add another way?

/be
Comment 19 Christian :Biesinger (don't email me, ping me on IRC) 2005-04-07 16:31:59 PDT
JS-impld QI used to |throw NS_ERROR_NO_INTERFACE;| until bug 243621 changed it
to set returnCode... I can't actually find out when it was checked in, the bug
doesn't say. bonsai indicates 2004-05-17 16:36.
Comment 20 neil@parkwaycc.co.uk 2005-04-07 16:35:55 PDT
(In reply to comment #18)
>What null test do you mean?
The one implied here, which answers my question:
>callers don't miss the lack of an error because the canonical JS QI
>returns null after setting Components.returnCode = NS_NOINTERFACE.
At least, JS callers don't. C++ callers can do, thus bug 288698.
Comment 21 Brendan Eich [:brendan] 2005-04-07 17:08:34 PDT
biesi: thanks, sorry -- I missed that.  So I hope we didn't actually break the
QI impls that throw Components.results.NS_NOINTERFACE -- rather we just want our
own code to use the method that's less painful to venkman.  Right?

Neil, anyone: mind if I dup bug 288698 against this bug?

/be
Comment 22 Brendan Eich [:brendan] 2005-04-17 15:17:37 PDT
*** Bug 288698 has been marked as a duplicate of this bug. ***
Comment 23 Brendan Eich [:brendan] 2006-01-17 12:02:37 PST
(In reply to comment #21)
> biesi: thanks, sorry -- I missed that.  So I hope we didn't actually break the
> QI impls that throw Components.results.NS_NOINTERFACE -- rather we just want our
> own code to use the method that's less painful to venkman.  Right?

Wrong.  I think bug 243621 was a bogus bug, its fix was half-assed anyway in light of C++ impls called by JS (won't Venkman see those QI failures as throws), and anyway we should not be hacking all our JS QI impls to work around a pain point to-do with Venkman.  Venkman should grow a feature to help ignore failing QIs appropriately.

From http://lxr.mozilla.org/mozilla/search?string=NS_NOINTERFACE it appears a number of JS QI impls are throwing anyway.  People must not be using Venkman and hitting these cases.

I'm going to fix this bug for 1.9 soon, and let the chips fall where they may -- requiring followup bugs on broken QI impls to be fixed.  I'll test startup and other common UI, but if I miss something, I'd like a chance to fix that rather than back this bug's patch out.

/be
Comment 24 neil@parkwaycc.co.uk 2006-01-17 16:49:20 PST
(In reply to comment #23)
>I think bug 243621 was a bogus bug, its fix was half-assed anyway in light
>of C++ impls called by JS (won't Venkman see those QI failures as >throws)
I didn't think that was the issue - 243621 was about JS impls called by C++.

>From http://lxr.mozilla.org/mozilla/search?string=NS_NOINTERFACE it appears a
>number of JS QI impls are throwing anyway.  People must not be using Venkman
>and hitting these cases.
Or most normal (i.e. excluding e.g. Component Viewer) JS QI impls succeed.
Comment 25 neil@parkwaycc.co.uk 2006-07-17 06:23:14 PDT
A related issue is that although you can of course still throw an nsresult to propagate it back as a return code it will log an exception in the console...
Comment 26 timeless 2007-03-11 04:29:19 PDT
Created attachment 258193 [details]
testcases

if we don't want to support returnCode, then we should include a warning whenever it's set. That's trivial to do.

Here's some code which is equivalent to evil C++ code. Unfortunately there still is random js which breaks every now and then.
Comment 27 timeless 2007-03-11 04:30:28 PDT
Created attachment 258194 [details] [diff] [review]
patch for thought

i'd like to know how much breaks with this. (this patch was written because extension manager in firefox used this magic and caused jesse and myself pain.)
Comment 28 Serge Gautherie (:sgautherie) 2007-08-11 15:05:14 PDT
(In reply to comment #27)
> extension manager in firefox used this magic and caused jesse and myself pain.)

SeaMonkey (extension manager) seems affected too: bug 384693.
Comment 29 Serge Gautherie (:sgautherie) 2007-10-25 06:40:44 PDT
(In reply to comment #28)
> SeaMonkey (extension manager) seems affected too: bug 384693.

Actually, bug 384693 was fixed by bug 393627.
Comment 30 neil@parkwaycc.co.uk 2007-10-25 08:50:43 PDT
Well, not exactly fixed... the correct fix would have been to fix Components.returnCode and use that instead.
Comment 31 Serge Gautherie (:sgautherie) 2007-10-25 10:10:47 PDT
("Agreed": let's say bug 393627 "wallpapered" the symptom;
current bug remains open to actually fix the cause.)
Comment 32 Brendan Eich [:brendan] 2009-01-20 14:35:19 PST
I'm not going to get to this any time soon. Someone with skills and time should grab this. It's still a bug, at least until we get rid of NS_COMFALSE and bad old nsIEnumerator interfaces (which we may already be almost free of -- I have not checked). Thoughts welcome from the cc: list.

/be
Comment 33 Mark Hammond [:markh] 2014-11-26 22:00:48 PST
Created attachment 8529506 [details] [diff] [review]
0001-Bug-287107-make-Components.returnCode-be-the-xpcom-n.patch

As discussed on https://groups.google.com/forum/#!topic/mozilla.dev.platform/xeCFgEPSl5g, Components.returnCode simply doesn't do anything at all these days.  The tree has changed significantly since this bug was looked at and the only current users of Components.returnCode are using it incorrectly anyway (they are throwing an integer after setting it)

The following patch fixes things so JS components that set Components.returnCode then return normally will have that returnCode passed back to their caller (and obviously will not report an exception as there is no exception - it's a successful return).

The fix is 1 line, but this patch also adds tests!  It creates a test c++ component that calls a test JS component.  This JS component uses Components.returnCode and the C++ code passes its nsresult back to the test harness.  Without the one line fix the test fails as the nsresult is NS_OK even when Components.returnCode is used.  With the fix the test passes.

Try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb0386f3e703
Comment 34 Bobby Holley (:bholley) (busy with Stylo) 2014-11-27 14:23:56 PST
Comment on attachment 8529506 [details] [diff] [review]
0001-Bug-287107-make-Components.returnCode-be-the-xpcom-n.patch

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

Thanks for the patch, and thanks twice for the tests!

::: js/xpconnect/idl/xpccomponents.idl
@@ +715,5 @@
>  
>      [implicit_jscontext]
>      readonly attribute jsval                            lastResult;
>      [implicit_jscontext]
> +    // A javascript component can set |returnCode| to specify an error code

I would say "an nsresult to be returned" rather than "an error code".

::: js/xpconnect/src/XPCWrappedJSClass.cpp
@@ -1402,5 @@
>              *((void**)p) = nullptr;
>          }
>      } else {
>          // set to whatever the JS code might have set as the result
> -        retval = pending_result;

Can you get rid of the pending_result value entirely?

It also seems like the current mechanism of clearing the PendingResult at the top of CallMethod will totally break in the context of nested calls. The correct solution seems to be to create a tiny RAII class called AutoSavePendingResult, whose constructor stashes the old value and sets it to NS_OK, and whose destructor restores the old value.

Test coverage for this would be nice, but only if it isn't a huge amount of trouble.

::: js/xpconnect/tests/unit/test_returncode.js
@@ +39,5 @@
> +               "exception caused NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS");
> +
> +  // wait for console messages to be delivered.
> +  while (thr.hasPendingEvents())
> +    thr.processNextEvent(true);

I'm not wild about spinning the event loop - can you turn this into an async xpcshell-test?
Comment 35 Mark Hammond [:markh] 2014-11-27 17:21:41 PST
Created attachment 8529981 [details] [diff] [review]
0001-Bug-287107-make-Components.returnCode-be-the-xpcom-n.patch

This is an update to the patch with most comments addressed (see below) - carrying r+ forward.

Thanks for the quick review!

(In reply to Bobby Holley (:bholley) from comment #34)
> I would say "an nsresult to be returned" rather than "an error code".

Fixed.

> Can you get rid of the pending_result value entirely?
> 
> It also seems like the current mechanism of clearing the PendingResult at
> the top of CallMethod will totally break in the context of nested calls. The
> correct solution seems to be to create a tiny RAII class called
> AutoSavePendingResult, whose constructor stashes the old value and sets it
> to NS_OK, and whose destructor restores the old value.

Please see the next patch.

> I'm not wild about spinning the event loop - can you turn this into an async
> xpcshell-test?

I worked out that I can do all this without a listener at all, so it's still a sync test but without event loop spinning.
Comment 36 Mark Hammond [:markh] 2014-11-27 17:38:30 PST
Created attachment 8529983 [details] [diff] [review]
0002-handle-nested-invocations.patch

This is a "part 2" only so the changes here are obvious - if this looks good I'll fold the patches before landing.

(In reply to Bobby Holley (:bholley) from comment #34)

> It also seems like the current mechanism of clearing the PendingResult at
> the top of CallMethod will totally break in the context of nested calls. The
> correct solution seems to be to create a tiny RAII class called
> AutoSavePendingResult, whose constructor stashes the old value and sets it
> to NS_OK, and whose destructor restores the old value.

I'm probably missing something, but the only issue I see here would be:

* C++ calls JS component.
* JS component sets .returnCode, but then calls *another* component which sets .returnCode to a different value.
* First JS component catches that error and returns.
* C++ caller sees the .returnCode set by the second component rather than the first.

If this is the scenario you are concerned about, I think this patch addresses it.  Instead of using a RAII class, it simply saves the pending_result across the invocation locally in XPCWrappedJSClass.cpp.  Without the change to XPCWrappedJSClass.cpp the new test fails, but passes with that change applied.

I apologize if I'm missing the point here (in which case you probably need to ELI5 ;)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=26aac8655d32
Comment 37 Mark Hammond [:markh] 2014-11-28 00:30:04 PST
Last try had some vs2013 and git->hg issues - greener one at https://tbpl.mozilla.org/?tree=Try&rev=ebb8ef89110d
Comment 38 Bobby Holley (:bholley) (busy with Stylo) 2014-12-07 15:53:11 PST
Comment on attachment 8529983 [details] [diff] [review]
0002-handle-nested-invocations.patch

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

This is right, yes. But the point of the RAII class is just to make sure that the code doesn't become incorrect if someone adds an early return.
Comment 39 Mark Hammond [:markh] 2014-12-07 19:56:12 PST
Created attachment 8533007 [details] [diff] [review]
0002-handle-nested-invocations.patch

(In reply to Bobby Holley (:bholley) from comment #38)
> This is right, yes. But the point of the RAII class is just to make sure
> that the code doesn't become incorrect if someone adds an early return.

Gotchya - so something like this?
Comment 40 Bobby Holley (:bholley) (busy with Stylo) 2014-12-08 00:04:34 PST
Comment on attachment 8533007 [details] [diff] [review]
0002-handle-nested-invocations.patch

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

Looks good! r=bholley

::: js/xpconnect/src/XPCWrappedJSClass.cpp
@@ +83,5 @@
>  }
>  
> +// A little stack-based RAII class to help management of the XPCContext
> +// PendingResult.
> +class MOZ_STACK_CLASS AutoPendingResult {

I'd call it AutoSavePendingResult.

@@ +96,5 @@
> +    ~AutoPendingResult() {
> +        m_xpcc->SetPendingResult(m_pending_result);
> +    }
> +private:
> +    XPCContext *m_xpcc;

mXPCContext

@@ +97,5 @@
> +        m_xpcc->SetPendingResult(m_pending_result);
> +    }
> +private:
> +    XPCContext *m_xpcc;
> +    nsresult m_pending_result;

mSavedResult
Comment 41 Mark Hammond [:markh] 2014-12-08 18:57:52 PST
Created attachment 8533489 [details] [diff] [review]
0001-Bug-287107-make-Components.returnCode-be-the-xpcom-n.patch

Rebased and folded version as landed, carrying r=bholley forward.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d97109e0c30
Comment 42 Mark Hammond [:markh] 2014-12-08 20:38:13 PST
Pushed a followup as the test failed on Android where the native test components aren't available.

https://hg.mozilla.org/integration/mozilla-inbound/rev/62bd0993a455

Note You need to log in before you can comment on or make changes to this bug.