Closed Bug 934421 Opened 7 years ago Closed 7 years ago

GenerationalGC: mochitest assertion following plugin tests

Categories

(Core :: Plug-ins, defect)

x86_64
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: jonco, Assigned: jonco)

References

Details

Attachments

(2 files)

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
Is there a prior test that actually uses plugins?  I wouldn't think that test_promise does.
(In reply to Andrew McCreight [:mccr8] from comment #1)

Yes indeed - this is directly after a whole suite of plugin tests.
Summary: GenerationalGC: mochitest test_promise.html failures → GenerationalGC: mochitest assertion following plugin tests
OS: Linux → Mac OS X
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.
Component: JavaScript Engine → Plug-ins
It looks like sJSObjWrappers hashtable in nsJSNPRuntime.cpp needs postbarriers.
Assignee: nobody → jcoppeard
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.
Attachment #828801 - Flags: review?(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 *.
Attachment #828801 - Flags: review?(terrence) → review+
(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().
Updated patch following review comments.
Attachment #829172 - Flags: review?(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.
Attachment #829172 - Flags: review?(bzbarsky) → review?(benjamin)
Attachment #829172 - Flags: review?(benjamin) → review?(jschoenick)
Comment on attachment 829172 [details] [diff] [review]
plugins-barriering v2

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

r=me on the plugin bits
Attachment #829172 - Flags: review?(jschoenick) → review+
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)
https://hg.mozilla.org/mozilla-central/rev/4e06fcbbba8e
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Depends on: 1013972
You need to log in before you can comment on or make changes to this bug.