Closed Bug 850293 Opened 7 years ago Closed 7 years ago

Remove nsScriptObjectHolder

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: terrence, Assigned: terrence)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch v0 (obsolete) — Splinter Review
The nsScriptObjectHolders, they do nothing!

They keep the wrapped object on the stack and so are protected by conservative GC. They also call JS_[Un]LockGCThing on the thing. This second rooting mechanism is far slower and needs to be removed for moving GC anyway. This patch simply replaces all instances of nsScriptObjectHolders with our new exact rooting classes.

This is my first real browser patch, so break out the fine-toothed comb.

Try run up at:
https://tbpl.mozilla.org/?tree=Try&rev=fbe0d70b9977
Attachment #724039 - Flags: review?(continuation)
\o/
Component: JavaScript Engine → DOM
QA Contact: general
Comment on attachment 724039 [details] [diff] [review]
v0

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

Followup for nsJSUtils::CompileFunction?

::: dom/base/nsJSEnvironment.cpp
@@ +1346,5 @@
>    if (NS_FAILED(rv)) {
>      return NS_ERROR_FAILURE;
>    }
>  
> +  aScriptObject.set(NULL); // ensure old object not used on failure...

nullptr
Question: do I need to update NS_ISCRIPTCONTEXT_IID and NS_PIDOMWINDOW_IID, since I changed a parameter to a method in them?
Attached patch v1 (obsolete) — Splinter Review
(In reply to Boris Zbarsky (:bz) from comment #4)
> Yes, you do.

Updated. Thanks Boris!
Attachment #724039 - Attachment is obsolete: true
Attachment #724039 - Flags: review?(continuation)
Attachment #724152 - Flags: review?(continuation)
Comment on attachment 724152 [details] [diff] [review]
v1

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

Hmm, looks like HoldScriptObject()/DropScriptObject() is only used by nsXULPrototypeCache.  Maybe that could be converted later into some more modern heap rooting mechanism.

sr? for the removal of nsScriptObjectHolder, and probably Smaug should also take a brief look at the changes to nsPIDOMWindow.h.  And possibly the rest. ;)

::: content/xbl/src/nsXBLPrototypeHandler.cpp
@@ +271,5 @@
>    nsIScriptContext *boundContext = boundGlobal->GetScriptContext();
>    if (!boundContext)
>      return NS_OK;
>  
> +  AutoPushJSContext cx(boundContext->GetNativeContext());

I don't know much about AutoPushJSContext, but this makes me a little nervous to move it across other push... stuff, which may be totally unrelated or may not.  Can't you just move |handler| down before right before EnsureEventHandler?
Attachment #724152 - Flags: superreview?(bugs)
Attachment #724152 - Flags: review?(continuation)
Attachment #724152 - Flags: review+
Comment on attachment 724152 [details] [diff] [review]
v1

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

Bobby might want to look at the AutoPushJSContext changes.
Attachment #724152 - Flags: feedback?(bobbyholley+bmo)
Attachment #724152 - Flags: superreview?(bugs) → superreview+
Duplicate of this bug: 698417
(In reply to Andrew McCreight [:mccr8] from comment #6)
> 
> Hmm, looks like HoldScriptObject()/DropScriptObject() is only used by
> nsXULPrototypeCache.  Maybe that could be converted later into some more
> modern heap rooting mechanism.

You are quite correct. This patch is for both exact rooting and to get us closer to being able to remove {Hold,Drop}ScriptObject in support of generational GC.
 
> sr? for the removal of nsScriptObjectHolder, and probably Smaug should also
> take a brief look at the changes to nsPIDOMWindow.h.  And possibly the rest.
> ;)

Who should I sr?
Attachment #724152 - Attachment is obsolete: true
Attachment #724152 - Flags: feedback?(bobbyholley+bmo)
Attachment #725008 - Flags: review?(bugs)
> Who should I sr?
Olli already sr+'d it.  Do you need a re-review on the rebased patch?  How much  changed?

As Ms2ger said, bholley should look at the AutoPushJSContext changes, though they seem less scary now.
Comment on attachment 725008 [details] [diff] [review]
v2: Applied review feedback and rebased.

Yeah, bholley should look at this too.
Attachment #725008 - Flags: review?(bugs) → review?(bobbyholley+bmo)
Comment on attachment 725008 [details] [diff] [review]
v2: Applied review feedback and rebased.

(In reply to Andrew McCreight [:mccr8] from comment #10)
> > Who should I sr?
> Olli already sr+'d it.  Do you need a re-review on the rebased patch?  How
> much  changed?

The change was tiny, I just missed his sr+ because I had the "Add an attachment" window open for too long. In the shell, doing one last test doesn't generally take enough time for two reviews to show up. :-)
 
> As Ms2ger said, bholley should look at the AutoPushJSContext changes, though
> they seem less scary now.
Attachment #725008 - Flags: superreview+
Comment on attachment 725008 [details] [diff] [review]
v2: Applied review feedback and rebased.

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

The AutoPushJSContext stuff looks fine.

In general, new consumers should almost always be using AutoJSContext. AutoPushJSContext was introduced to mitigate regression risk from random dependencies on a specific JSContext. But for unless you a very good reason why you need one JSContext in particular, we should use AutoJSContext, because that maps directly to the single-JSContext world. But both of these cases appear to be moving an existing AutoPushJSContext declaration earlier in the function, which is fine.

Let me know if there's anything else I should look at.
Attachment #725008 - Flags: review?(bobbyholley+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/a0aa85db5d17
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.