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

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 on attachment 178145 [details] [diff] [review] proposed fix Oops, missed the IDispatch stuff that's configured off by default. /be

Comment on attachment 178179 [details] [diff] [review] fix, v2 sr=jst

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.

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 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

Checked in. Someone nominate for branches if there's a good reason. /be

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.

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

Note bug 287846, another regression caused by the fix for this bug.

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

Hermann's right, reopening. /be

Taking. /be

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

(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?

(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

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.

(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.

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

*** Bug 288698 has been marked as a duplicate of this bug. ***

(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

(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.

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...

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.

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.)

(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.

(In reply to comment #28) > SeaMonkey (extension manager) seems affected too: bug 384693. Actually, bug 384693 was fixed by bug 393627.

Well, not exactly fixed... the correct fix would have been to fix Components.returnCode and use that instead.

("Agreed": let's say bug 393627 "wallpapered" the symptom; current bug remains open to actually fix the cause.)

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

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 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?

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.

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

Last try had some vs2013 and git->hg issues - greener one at https://tbpl.mozilla.org/?tree=Try&rev=ebb8ef89110d

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.

(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 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

Rebased and folded version as landed, carrying r=bholley forward. https://hg.mozilla.org/integration/mozilla-inbound/rev/6d97109e0c30

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

https://hg.mozilla.org/mozilla-central/rev/6d97109e0c30 https://hg.mozilla.org/mozilla-central/rev/62bd0993a455