952840 - Remove a few gotos in XPConnect

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Andrew McCreight [:mccr8]]

There are a few gotos sitting around that are fairly gratuitous.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Eliminate goto in XPCWrappedNative::FindTearOff.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Get rid of one goto in XPCNativeSet::GetNewOrUsed.

arr.get() only returns null if the second argument to its constructor is null,
but here it is being infallibly allocated.

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: part 3 - Get rid of goto in nsXPConnect::CheckForDebugMode.

Rather than checking one condition, doing a goto on failure, then checking another
condition and doing a goto on failure, just check both conditions at once.

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8351208 [details] [diff] [review]
part 3 - Get rid of goto in nsXPConnect::CheckForDebugMode.

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

Would be better to swap the cases around, I think. It's not really common to have the failure case run off the end of the function.

::: js/xpconnect/src/nsXPConnect.cpp
@@ +1218,5 @@
>      const char jsdServiceCtrID[] = "@mozilla.org/js/jsd/debugger-service;1";
>      nsCOMPtr<jsdIDebuggerService> jsds = do_GetService(jsdServiceCtrID, &rv);
> +    if (NS_SUCCEEDED(rv) && JS_SetDebugModeForAllCompartments(cx, gDesiredDebugMode)) {
> +        if (gDesiredDebugMode) {
> +            rv = jsds->ActivateDebugger(rt);

Dead assignment

Comment 5

[Andrew McCreight [:mccr8]]

Comment on attachment 8351208 [details] [diff] [review]
part 3 - Get rid of goto in nsXPConnect::CheckForDebugMode.

Good points.

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 8351206 [details] [diff] [review]
part 1 - Eliminate goto in XPCWrappedNative::FindTearOff.

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

::: js/xpconnect/src/XPCWrappedNative.cpp
@@ +1513,5 @@
>          to = newChunk->mTearOffs;
>      }
>  
>      {
> +        // Scope keeps |tearoff| from leaking across the rest of the function.

Yes, but this isn't really relevant anymore. There's not really any harm in just unscoping this AFAICT. Can you do that?

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 8351207 [details] [diff] [review]
part 2 - Get rid of one goto in XPCNativeSet::GetNewOrUsed.

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

This makes me lol. Blame says this was either jst or jband. Let's say it was jband. ;-)

Comment 8

[Andrew McCreight [:mccr8]]

Attachment: part 3 - Get rid of goto in nsXPConnect::CheckForDebugMode.

Rather than checking one condition, doing a goto on failure, then checking another
condition and doing a goto on failure, just check both conditions at once.

Comment 9

[Andrew McCreight [:mccr8]]

As noted by Ms2ger, we're ignoring failures of ActivateDebugger.  Steve, should we not do that?  What should happen in the case of those failures, the same stuff as we do in the other failure cases?

Comment 10

[Steve Fink [:sfink] [:s:]]

Ignoring it looks bad. I think you need to JS_SetDebugModeForAllCompartments(cx, false) in that case, then do the rest of the current failure handling. You can ignore the return value of JS_SetDebugModeForAllCompartments when turning it off for error handling.

Comment 11

[Andrew McCreight [:mccr8]]

I'll file a followup to deal with the debug stuff, as it will affect how I want to eliminate the goto.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2cb3e4d522cc
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/64cfe4a6eafa

Comment 12

[Andrew McCreight [:mccr8]]

Woops, forgot to unscope.  Followup:
https://hg.mozilla.org/integration/mozilla-inbound/rev/99eb8a370b21

Comment 13

[Carsten Book [:Tomcat]]

had to backout this changes for testfailures like  https://tbpl.mozilla.org/php/getParsedLog.php?id=32623378&tree=Mozilla-Inbound

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2cb3e4d522cc
https://hg.mozilla.org/mozilla-central/rev/64cfe4a6eafa

Comment 15

[Andrew McCreight [:mccr8]]

So it was actually only the unscoping that got backed out, it looks like:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/ce408ac339c5
Somehow this was causing permaorange in B2G ICS Emulator Opt M2.

The failures are:
2050 ERROR TEST-UNEXPECTED-FAIL | /tests/content/events/test/test_bug864040.html | TEXTAREA: failed to set selection
2058 ERROR TEST-UNEXPECTED-FAIL | /tests/content/events/test/test_bug864040.html | DIV: failed to set selection
01-07 15:30:51.987 706 706 I GeckoDump: 2050 ERROR TEST-UNEXPECTED-FAIL | /tests/content/events/test/test_bug864040.html | TEXTAREA: failed to set selection
01-07 15:30:52.827 706 706 I GeckoDump: 2058 ERROR TEST-UNEXPECTED-FAIL | /tests/content/events/test/test_bug864040.html | DIV: failed to set selection

I guess the destructor for AutoMarkingWrappedNativeTearOffPtr is being called after we decide what to return, so we end up returning the wrong value.  I was wondering about that, but I guess I should have looked it up before changing it.

Comment 16

[Andrew McCreight [:mccr8]]

Filed bug 957191 for the part 3 stuff.