934421 - GenerationalGC: mochitest assertion following plugin tests

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Jon Coppeard (:jonco)]

In GGC browser builds mochitest /tests/dom/promise/tests/test_promise.html fails, on MacOSX only:

16:07:23     INFO -  26412 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/promise/tests/test_promise.html | Assertion count 1 is greater than expected range 0-0 assertions.

Full log: https://tbpl.mozilla.org/php/getParsedLog.php?id=29997280&full=1&branch=try#error0

I think this is the assertion:

16:07:23     INFO -  [Parent 879] ###!!! ASSERTION: Uh, hash not empty?: 'sJSObjWrappers.entryCount == 0', file ../../../../dom/plugins/base/nsJSNPRuntime.cpp, line 241

Comment 1

[Andrew McCreight [:mccr8]]

Is there a prior test that actually uses plugins?  I wouldn't think that test_promise does.

Comment 2

[Jon Coppeard (:jonco)]

(In reply to Andrew McCreight [:mccr8] from comment #1)

Yes indeed - this is directly after a whole suite of plugin tests.

Comment 3

[Andrew McCreight [:mccr8]]

In general, I'd say it would be worthwhile filing all of these mochitest GGC bugs in their respective components, so you can possibly get help from domain experts, even if it ends up being a problem in GGC itself.  "Hg annotations" on the MXR for a file can be useful to figure out what component that is.

Comment 4

[Jon Coppeard (:jonco)]

It looks like sJSObjWrappers hashtable in nsJSNPRuntime.cpp needs postbarriers.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: plugins-barriering

Here's a patch to postbarrier the keys of the plugin wrapper table.  PLDHash doesn't work for this because there's no way to infalliably rekey entries, so I changed the implementation to use js::HashTable.

I'm going to ask terrence for review first, and then ask a browser person for review after that.

Comment 6

[Terrence Cole [:terrence]]

Comment on attachment 828801 [details] [diff] [review]
plugins-barriering

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

(In reply to Jon Coppeard (:jonco) from comment #5)
> 
> I'm going to ask terrence for review first, and then ask a browser person
> for review after that.

Good plan.

r=me for the JS hashtable bits and afaict the rest maintains the existing behavior.

::: dom/plugins/base/nsJSNPRuntime.cpp
@@ +30,1 @@
>  

Extra newline.

@@ +516,5 @@
>    OnWrapperDestroyed();
>  }
>  
> +void
> +nsJSObjWrapper::ClearJSObject() {

{ on new line, I think.

@@ +1007,1 @@
>        NS_ERROR("Error initializing PLDHashTable!");

I guess at least the typename should change, although better would be to change the error to something that's actually useful while you're here.

@@ +1013,5 @@
>    nsJSObjWrapperKey key(obj, npp);
>  
> +  JSObjWrapperTable::AddPtr p = sJSObjWrappers.lookupForAdd(key);
> +
> +  if (p/* && p->value*/) {

I guess you can remove this comment, if the following assert isn't triggering on Try.

@@ +1015,5 @@
> +  JSObjWrapperTable::AddPtr p = sJSObjWrappers.lookupForAdd(key);
> +
> +  if (p/* && p->value*/) {
> +    MOZ_ASSERT(p->value);
> +    // Found a live nsJSObjWrapper, return it.

This comment seems pretty superfluous.

@@ +1026,5 @@
> +  nsJSObjWrapper *wrapper =
> +    (nsJSObjWrapper *)_createobject(npp, &sJSObjWrapperNPClass);
> +
> +  if (!wrapper) {
> +    // Out of memory, entry not yet added to table.

This comment is more confusing than anything.

@@ +1033,5 @@
>  
> +  wrapper->mJSObj = obj;
> +
> +  if (!sJSObjWrappers.add(p, key, wrapper)) {
> +    // Out of memory, free the wrapper we created.

Likewise for this one.

@@ +1878,5 @@
> +      if (npobj->mNpp == npp) {
> +        npobj->ClearJSObject();
> +        _releaseobject(npobj);
> +        e.removeFront();
> +      }

This doesn't seem to call invalidate?

::: dom/plugins/base/nsJSNPRuntime.h
@@ +34,2 @@
>  
> +  JSObject * mJSObj;

Extra space after *.

Comment 7

[Jon Coppeard (:jonco)]

(In reply to Terrence Cole [:terrence] from comment #6)

Thanks for the comments!

> @@ +1878,5 @@
> > +      if (npobj->mNpp == npp) {
> > +        npobj->ClearJSObject();
> > +        _releaseobject(npobj);
> > +        e.removeFront();
> > +      }
> 
> This doesn't seem to call invalidate?

Invalidate just calls ClearJSObject() and removes it from the hashtable.  This is going to remove it from the hashtable anyway, so it just calls ClearJSObject().

Comment 8

[Jon Coppeard (:jonco)]

Attachment: plugins-barriering v2

Updated patch following review comments.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 829172 [details] [diff] [review]
plugins-barriering v2

Benjamin, please pass this back if you think I should be reviewing this, but this seems possibly more up your alley.

Comment 10

[John Schoenick [:johns]]

Comment on attachment 829172 [details] [diff] [review]
plugins-barriering v2

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

r=me on the plugin bits

Comment 11

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0a641abef602

Comment 12

[Ed Morley [:emorley]]

Unfortunately this and the other bugs in https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=db0f8a5eeb33 have been backed out for causing rootanalysis assertions, eg:
https://tbpl.mozilla.org/php/getParsedLog.php?id=30835010&tree=Mozilla-Inbound

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=05a0228c2caa

(For quick relanding, I recommend the third party qbackout extension and '--apply' mode)

Comment 13

[Jon Coppeard (:jonco)]

Relanding:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e06fcbbba8e

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4e06fcbbba8e