Closed Bug 952840 Opened 7 years ago Closed 7 years ago

Remove a few gotos in XPConnect

Categories

(Core :: XPConnect, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

Attachments

(3 files, 1 obsolete file)

There are a few gotos sitting around that are fairly gratuitous.
arr.get() only returns null if the second argument to its constructor is null,
but here it is being infallibly allocated.
Attachment #8351207 - Flags: review?(bobbyholley+bmo)
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.
Attachment #8351208 - Flags: review?(bobbyholley+bmo)
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 on attachment 8351208 [details] [diff] [review]
part 3 - Get rid of goto in nsXPConnect::CheckForDebugMode.

Good points.
Attachment #8351208 - Flags: review?(bobbyholley+bmo)
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?
Attachment #8351206 - Flags: review?(bobbyholley+bmo) → review+
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. ;-)
Attachment #8351207 - Flags: review?(bobbyholley+bmo) → review+
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.
Attachment #8351208 - Attachment is obsolete: true
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?
Flags: needinfo?(sphink)
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.
Flags: needinfo?(sphink)
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
Attachment #8355279 - Flags: checkin-
https://hg.mozilla.org/mozilla-central/rev/2cb3e4d522cc
https://hg.mozilla.org/mozilla-central/rev/64cfe4a6eafa
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Filed bug 957191 for the part 3 stuff.
Blocks: 1155764
You need to log in before you can comment on or make changes to this bug.