Closed Bug 792861 Opened 7 years ago Closed 7 years ago

Make HoldJSObjects/DropJSObjects infallible

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

Attachments

(1 file, 3 obsolete files)

Followup from bug 782792. Now that mJSHolders is infallible, we should be able to get rid of a bunch of nsresult error propagation. I probably should have done this at the same time, but oh well.
Assignee: nobody → continuation
Status: NEW → ASSIGNED
Summary: Remove useless nsresults from mJSHolders functions → Make HoldJSObjects/DropJSObjects infallible
Comment on attachment 673914 [details] [diff] [review]
Make HoldJSObjects/DropJSObjects infallible

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

::: content/base/src/nsContentUtils.cpp
@@ +4561,5 @@
> +  NS_LOG_ADDREF(sXPConnect, sJSGCThingRootCount, "HoldJSObjects",
> +                sizeof(void*));
> +
> +  if (NS_FAILED(sXPConnect->AddJSHolder(aScriptObjectHolder, aTracer))) {
> +    MOZ_CRASH();

Just use MOZ_ALWAYS_TRUE.
Blocks: 804220
Blocks: 780758
(In reply to :Ms2ger from comment #2)
> Just use MOZ_ALWAYS_TRUE.
That won't crash in release builds.
Why should it? It can never happen.
It is just defense-in-depth against future changes. Failures here tend to be dangerous and hard to find in testing.
Comment on attachment 673914 [details] [diff] [review]
Make HoldJSObjects/DropJSObjects infallible

Feel free to bounce this over to Olli or somebody else if you don't think you are the right person to review this. Nothing very complex here, just clearing out a pipeline...
Attachment #673914 - Flags: review?(bobbyholley+bmo)
Comment on attachment 673914 [details] [diff] [review]
Make HoldJSObjects/DropJSObjects infallible

If they're infallible, can we just mark them as [notxpcom] in nsIXPConnect and have them be bonafide void methods?

Feel free to reflag if there's a good reason not to do that.
Attachment #673914 - Flags: review?(bobbyholley+bmo)
I wonder if it would be a nice idea to make the XPC hold/drop functions return failure if the object is/isn't present. Right now, we could end up in a situation where sJSGCThingRootCount is wrong if a class does HOLD or DROP too much. I think that would require an extra hashtable lookup.
Attached patch rework HOLD/DROP (obsolete) — Splinter Review
Reworked version that cleans up the HOLD/DROP a bit. We check for HOLD of things we already hold and DROP of things we don't hold, and don't adjust ref counts in case we did it badly.

As an experiment, I threw in assertions that we never fail these checks:
  https://tbpl.mozilla.org/?tree=Try&rev=9aa3ce78d219

In practice, we may want to just deal with the root count separately from the infallibillization, which is mostly a side show here.
Attachment #673914 - Attachment is obsolete: true
This is a little cleaner after Olli's fixup of the root counting mess. Per Bobby's suggestion, I made the rooting functions [notxpcom]. I'm not sure why I didn't try that before, it seems to work just fine. I guess
Attachment #680903 - Attachment is obsolete: true
Attachment #686730 - Attachment is obsolete: true
Comment on attachment 686790 [details] [diff] [review]
Make HoldJSObjects/DropJSObjects infallible

L64 try run looks good. Olli is an expert on this code now, so I'll make him review. :)
Attachment #686790 - Flags: review?(bugs)
Comment on attachment 686790 [details] [diff] [review]
Make HoldJSObjects/DropJSObjects infallible


> nsContentUtils::HoldJSObjects(void* aScriptObjectHolder,
>                               nsScriptObjectTracer* aTracer)
> {
>-  NS_ENSURE_TRUE(sXPConnect, NS_ERROR_UNEXPECTED);
>-
>-  return sXPConnect->AddJSHolder(aScriptObjectHolder, aTracer);
>+  sXPConnect->AddJSHolder(aScriptObjectHolder, aTracer);
> }
Please don't remove the null check.
perhaps
if (!sXPConnect) {
  MOZ_ASSERT(false, "Unexpected time to call HoldJSObjects");
  return;
}
I know this is a bit ugly, but I'm worried about extra crashes during shutdown.
Could we at least have the null check for some time.



>-    [noscript] void addJSHolder(in voidPtr aHolder,
>-                                in nsScriptObjectTracerPtr aTracer);
>+    [noscript,notxpcom] void addJSHolder(in voidPtr aHolder,
>+                                         in nsScriptObjectTracerPtr aTracer);
> 
>     /**
>      * Stop rooting the JS objects held by aHolder.
>      * @param aHolder The object that hold the rooted JS objects.
>      */
>-    [noscript] void removeJSHolder(in voidPtr aHolder);
>+    [noscript,notxpcom] void removeJSHolder(in voidPtr aHolder);

This shouldn't affect whether binary addons can call these methods, so should be ok.
Attachment #686790 - Flags: review?(bugs) → review+
> I know this is a bit ugly, but I'm worried about extra crashes during shutdown.
> Could we at least have the null check for some time.

Sure, I'll do something like you suggest. I guess if we try to root something after XPConnect has gone away, we hopefully don't actually have any JS things any more so it shouldn't matter, and there's probably not a lot going on that is attacker-controllable anyways.
I added a null check and assertion for nsXPConnect in HoldJSObjects.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ecbc55bdcb05
https://hg.mozilla.org/mozilla-central/rev/ecbc55bdcb05
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.