Status

()

defect
--
critical
RESOLVED FIXED
7 years ago
4 months ago

People

(Reporter: mccr8, Assigned: smaug)

Tracking

19 Branch
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 9 obsolete attachments)

In bug 792861, I have a patch that checks that
a) HoldJSObject is only done when something isn't in the JS holders table already
b) DropJSObjects is only done when something is in the JS holders table

https://tbpl.mozilla.org/?tree=Try&rev=9aa3ce78d219

M1, M-C, J: tries to DropJSObjects an object that isn't in the holders table
  nsContentUtils::ReleaseWrapper
  nsBaseContentList::cycleCollection::UnlinkImpl

M2, X: tries to Hold something already held object
  nsContentUtils::PreserveWrapper
  nsEventTargetSH::AddProperty

M3, M4, R are okay

M5 tries to drop something that isn't held
  nsContentUtils::ReleaseWrapper
  nsComputedDOMStyle::cycleCollection::UnlinkImpl

bc drops an unheld thing
  nsContentUtils::ReleaseWrapper
  nsDOMCSSAttributeDeclaration::cycleCollection::UnlinkImpl

C drops an unheld thing
  nsContentUtils::ReleaseWrapper
  mozilla::css::DOMCSSStyleRule::cycleCollection::UnlinkImpl

So it looks like it is a widespread enough problem we should just bail on it.

Another approach would just be to release layout statics when the mJSHolders table is empty, and hold it when it becomes non-empty, but that could be annoying in its own way.
Could we try having hold/drop increment/decrement a counter in the table instead of just adding/removing?
I think the "counter in the table" is just going to be the size of the table, so there's no real need to explicitly maintain a separate one.
The benefit of doing counter in table (per entry, note!) is that doing hold, hold, drop, drop in that order is no longer a footgun...
(In reply to Andrew McCreight [:mccr8] from comment #0)
> Another approach would just be to release layout statics when the mJSHolders
> table is empty, and hold it when it becomes non-empty.
I like this approach.
(In reply to Boris Zbarsky (:bz) from comment #3)
> The benefit of doing counter in table (per entry, note!) is that doing hold,
> hold, drop, drop in that order is no longer a footgun...

Oh I see what you mean. Still, that won't help with things like hold, hold, drop. The minimum requirement here is that we keep XPConnect alive while there are still things in the table. Do you want any further checking of hold/drop beyond that?
> Still, that won't help with things like hold, hold, drop.

Help in what sense?

Right now, actually dropping in anything other than unlink or your destructor is a footgun of ginormous proportions, because we have macros for wrappercached stuff that hide hold/drop calls.  So you might not even realize that you've done "hold hold drop", and will be holding garbage JS object pointers.  What I'd like, ideally, is a way to not have that footgun.

But yes, that can be a separate issue from just keeping xpconnect alive...
Ideally, we'd NULL out all JS pointers when we do the DROP, but I can't think of any great way to do that aside from creating a new callback that is to Trace what Unlink is to Traverse, which is awful in its own way.

In practice, we're probably mostly okay, because Unlinked objects are usually only going to run their destructor.
> Ideally, we'd NULL out all JS pointers when we do the DROP

How does that work with inheritance?

In fact, how does the whole hold/drop thing work with inheritance?
Blocks: 811369
No longer blocks: 811369
Blocks: 811369
Assignee: nobody → continuation
I'm not going to be able to get to this right away.
Assignee: continuation → nobody
Assignee: nobody → bugs
Blocks: 808736
Posted patch WIP, assertions (obsolete) — Splinter Review
This catches quite some cases I'll need to change.
Posted patch fixes, WIP (obsolete) — Splinter Review
Let's see if something else need to be still fixed.
https://tbpl.mozilla.org/?tree=Try&rev=0ca477b9d082

dtors may cause problems, and if so, I may need to add some flag to not
do the tracing when drop is calling in dtor.
Comment on attachment 683074 [details] [diff] [review]
fixes, WIP

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

::: content/media/webaudio/AudioBuffer.cpp
@@ +53,5 @@
>  }
>  
>  AudioBuffer::~AudioBuffer()
>  {
> +  mChannels.Clear();

You probably want to do this in Unlink, too, to break any potential cycles.

::: dom/base/nsJSTimeoutHandler.cpp
@@ +151,2 @@
>      } else {
>        NS_WARNING("No func and no expr - roots may not have been removed");

This last case looks impossible, so you could eliminate it and refactor a little bit.
Posted patch fixes, WIP (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=adc138e68462
Attachment #683074 - Attachment is obsolete: true
IDB needs still some fixes...
Posted patch fixes, WIP, v4 (obsolete) — Splinter Review
Getting closer...
Attachment #683151 - Attachment is obsolete: true
Attachment #682557 - Flags: review?(continuation)
Comment on attachment 682638 [details] [diff] [review]
WIP, assertions

Not super pretty but works.
Attachment #682638 - Flags: review?(continuation)
Comment on attachment 683788 [details] [diff] [review]
fixes, WIP, v4

IDBRequest requires that wrapper is caches when mResultVal is set.
Wrapper cache is cleared in nsDOMEventTargetHelper (which IDBWrapperCache inherits)
Attachment #683788 - Flags: review?(continuation)
Comment on attachment 682557 [details] [diff] [review]
WIP - base, without assertions

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

Can't you just do the nsLayoutStatics::AddRef/Release inside of XPCJSRuntime? Then you'd avoid having to thread the boolean all over the place.

Also, please remove the declarations of sJSGCThingRootCount from nsContentUtils.h and nsContentUtils.cpp.
Attachment #682557 - Flags: review?(continuation)
(In reply to Andrew McCreight [:mccr8] from comment #27)
> Can't you just do the nsLayoutStatics::AddRef/Release inside of
> XPCJSRuntime? Then you'd avoid having to thread the boolean all over the
> place.
Hmm, that is indeed better.


> Also, please remove the declarations of sJSGCThingRootCount from
> nsContentUtils.h and nsContentUtils.cpp
Uh, how did I forgot that. I was going to do it.
Posted patch baseSplinter Review
Attachment #682557 - Attachment is obsolete: true
Attachment #684152 - Flags: review?(continuation)
Posted patch assert (obsolete) — Splinter Review
Attachment #682638 - Attachment is obsolete: true
Attachment #682638 - Flags: review?(continuation)
Attachment #684153 - Flags: review?(continuation)
Comment on attachment 684152 [details] [diff] [review]
base

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

Thanks, this is much nicer.

You still forgot to remove the declarations of sJSGCThingRootCount from nsContentUtils.h and nsContentUtils.cpp. :)  r=me with that.
Attachment #684152 - Flags: review?(continuation) → review+
Attachment #684166 - Flags: review?(continuation)
Attachment #683788 - Attachment is obsolete: true
Attachment #683788 - Flags: review?(continuation)
Comment on attachment 684153 [details] [diff] [review]
assert

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

I don't really like how spread all over the place this ends up being.

You could move the call of AssertNoObjectsToTrace to XPCJSRuntime::RemoveJSHolder, move all of the supporting stuff from nsContentUtils to XPCJSRuntime, and make the interface AssertNoObjectsToTrace instead of GetJSHolderTracer, and you'd only have to expose AssertNoObjectsToTrace on nsCycleCollectionJSRuntime instead of nsIXPConnect. nsCycleCollector could call it by doing
if (mJSRuntime)
  mJSRuntime->AssertNoObjectsToTrace(...)

How does that sound to you?

::: content/base/src/nsContentUtils.cpp
@@ +4521,5 @@
>    return sXPConnect->AddJSHolder(aScriptObjectHolder, aTracer);
>  }
>  
> +#ifdef DEBUG
> +extern void* gCycleCollectorUnlinkObject;

You should add a comment about where this is ultimately defined.

@@ +4532,5 @@
> +}
> +
> +void AssertNoObjectsToTrace(void* aPossibleJSHolder)
> +{
> +  if (nsContentUtils::XPConnect()) {

Maybe use an early return here?

@@ +4547,5 @@
>  nsresult
>  nsContentUtils::DropJSObjects(void* aScriptObjectHolder)
>  {
> +#ifdef DEBUG
> +  if (aScriptObjectHolder != gCycleCollectorUnlinkObject) {

If you set gCycleCollectorUnlinkObject to null before the call to AssertNoObjectsToTrace in CollectWhite, then you can move this gCycleCollectorUnlinkObject check inside AssertNoObjectsToTrace.

You should add a comment about why this check is needed.
Attachment #684153 - Flags: review?(continuation)
Posted patch assert (obsolete) — Splinter Review
Attachment #685303 - Flags: review?(continuation)
Attachment #684166 - Attachment is obsolete: true
Attachment #684166 - Flags: review?(continuation)
Attachment #685304 - Flags: review?(continuation)
Attachment #684153 - Attachment is obsolete: true
Comment on attachment 685303 [details] [diff] [review]
assert

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

Thanks, this is a lot nicer!

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +281,5 @@
>  {
> +#ifdef DEBUG
> +    // Assert that the holder doesn't try to keep any GC things alive.
> +    // In case of unlinking cycle collector calls AssertNoObjectsToTrace
> +    // manually.

If you could throw something at the end here like "because we don't want to check the holder before we are finished unlinking it" that would be good.

::: js/xpconnect/src/xpcprivate.h
@@ +485,5 @@
>      static XPCJSRuntime* GetRuntimeInstance();
>      XPCJSRuntime* GetRuntime() {return mRuntime;}
>  
> +#ifdef DEBUG
> +    void ObjectToUnlink(void* aObject);

Should this be called something like SetObjectToUnlink?

@@ +830,5 @@
>      nsresult AddJSHolder(void* aHolder, nsScriptObjectTracer* aTracer);
>      nsresult RemoveJSHolder(void* aHolder);
>      nsresult TestJSHolder(void* aHolder, bool* aRetval);
> +#ifdef DEBUG
> +    void ObjectToUnlink(void* aObject) { mObjectToUnlink = aObject; }

Likewise.

::: xpcom/base/nsCycleCollector.cpp
@@ +2382,5 @@
>      for (uint32_t i = 0; i < count; ++i) {
>          PtrInfo *pinfo = mWhiteNodes->ElementAt(i);
> +#ifdef DEBUG
> +        if (mJSRuntime)
> +            mJSRuntime->ObjectToUnlink(pinfo->mPointer);

nit: brace the body of the if
Attachment #685303 - Flags: review?(continuation) → review+
Comment on attachment 685304 [details] [diff] [review]
fixes, v6, removes also sJSGCThingRootCount

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

The IndexedDB changes are a little more dramatic than the rest, maybe it would be good to get an IDB peer to glance at them.

::: dom/base/nsJSTimeoutHandler.cpp
@@ +140,5 @@
>  
>  void
>  nsJSScriptTimeoutHandler::ReleaseJSObjects()
>  {
>    if (mExpr || mFunObj) {

nit: I think you could reorganize the body of this |if| to be:
  if (mExpr) {
    mExpr = nullptr;
  } else {
    mFunObj = nullptr;
  }
  NS_DROP_JS_OBJECTS(this,nsJSScriptTimeoutHandler);

The NS_WARNING case will never be hit, and you might as well factor out the DROP to be less confusing. Not a big deal, it is just a little confusing as is.
Attachment #685304 - Flags: review?(continuation) → review+
Comment on attachment 685304 [details] [diff] [review]
fixes, v6, removes also sJSGCThingRootCount

Kyle, could you look at the IDB changes?
The idea is to assert that after drop/unlink there is nothing to trace.
Attachment #685304 - Flags: review?(khuey)
It might be a good idea to land the fixes earlier in the stack than the assertions, to avoid causing bisections to blow up. Hopefully that won't be that hard.
Comment on attachment 685304 [details] [diff] [review]
fixes, v6, removes also sJSGCThingRootCount

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

::: dom/indexedDB/IDBCursor.cpp
@@ +427,5 @@
>    return NS_OK;
>  }
>  
> +void
> +IDBCursor::DropJSObjects()

I would prefer that this function went after the dtor (so the header order matches the cpp order to the extent possible).

@@ +429,5 @@
>  
> +void
> +IDBCursor::DropJSObjects()
> +{
> +  if (mRooted) {

Can we just do

if (!mRooted) {
  return;
}

?

::: dom/indexedDB/IDBKeyRange.cpp
@@ +308,5 @@
>  
> +void
> +IDBKeyRange::DropJSObjects()
> +{
> +  if (mRooted) {

Same here, I think we should early return.

::: dom/indexedDB/IDBTransaction.cpp
@@ -189,5 @@
>      mActorChild->Send__delete__(mActorChild);
>      NS_ASSERTION(!mActorChild, "Should have cleared in Send__delete__!");
>    }
> -
> -  nsContentUtils::ReleaseWrapper(static_cast<nsIDOMEventTarget*>(this), this);

There's one of these in IDBDatabase too.
Attachment #685304 - Flags: review?(khuey) → review+
Posted patch assert, v5Splinter Review
Attachment #685303 - Attachment is obsolete: true
Attachment #685304 - Attachment is obsolete: true
This is the same as for m-c, but with the null check for xpconnect I added in
"fixes".

https://tbpl.mozilla.org/?tree=Try&rev=dee2522a8d13
Attachment #686139 - Flags: review?(continuation)
Attachment #686139 - Flags: review?(continuation) → review+
Comment on attachment 686139 [details] [diff] [review]
Base patch for Aurora

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Old problem which is getting
worse the more we add new APIs
User impact if declined: Possibly odd shutdown crashes
Testing completed (on m-c, etc.): landed today
Risk to taking this patch (and alternatives if risky): Should be relatively safe
String or UUID changes made by this patch: NA

I'd like to take the patch in order to avoid problems like bug 810618.
Attachment #686139 - Flags: approval-mozilla-aurora?
Comment on attachment 686139 [details] [diff] [review]
Base patch for Aurora

Without having a specific unresolved crasher that this prevents, let's instead let this ride the trains.
Attachment #686139 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Blocks: 848153
(In reply to Alex Keybl [:akeybl] from comment #47)
> Comment on attachment 686139 [details] [diff] [review]
> Base patch for Aurora
> 
> Without having a specific unresolved crasher that this prevents, let's
> instead let this ride the trains.

Now we have a bad crash in bug 848153.
Severity: normal → critical
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.