1262050 - Frequent "Expected matching plugin instance: 'parentInstance'" assertions when running dom/plugins tests

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

I've been trying to track down a recent surge in Windows debug test_fullscreen-api.html "TEST-UNEXPECTED-ERROR | dom/html/test/test_fullscreen-api.html | Assertion count 2 is greater than expected range 0-1 assertions." failures on Ash. It appears that the test was previously annotated by jgriffin for hitting this assert, and we're now just hitting it twice intermittently instead.
https://hg.mozilla.org/mozilla-central/rev/843f2db83213

Looking at the changeset, I see a number of other dom/plugins assertion annotations added and have confirmed in the logs that it's the same assertion being hit for them.

For now, I intend to bump the allowance in test_fullscreen-api.html to 2, but it would be good to know what the severity of this is. Jim, I see that you added the assertion back in bug 1132874. What's your take?

Comment 1

[Ryan VanderMeulen [:RyanVM]]

List of tests that hit this:
dom/html/test/test_fullscreen-api.html
dom/plugins/test/mochitest/test_GCrace.html
dom/plugins/test/mochitest/test_defaultValue.html
dom/plugins/test/mochitest/test_instance_re-parent.html
dom/plugins/test/mochitest/test_propertyAndMethod.html
dom/plugins/test/mochitest/test_windowed_invalidate.html
dom/plugins/test/mochitest/test_zero_opacity.html
layout/generic/test/test_plugin_focus.html

Comment 2

[Jim Mathies [:jimm]]

We can remove the second assertion and remove any assertion count annotations we've added for it. 

http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentChild.cpp#3083

tracy, can you take this on? You'll have to get with RyanVM on figuring out how to remove the annotations.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

I was just cooking up a patch to add more information to the tests pointing at this bug, so I can do the removal if you want.

Comment 4

[Tracy Walker [:tracy]]

Sure, go ahead, I'll follow along for the learning.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

jgriffin's patch also annotated a couple tests that don't appear to be asserting anymore:
dom/plugins/test/mochitest/test_crashing.html
dom/plugins/test/mochitest/test_redirect_handling.html

I'm going to try removing those while I'm at it.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Attachment: remove the unneeded assert and associated test annotations

Try says this is good with and without e10s enabled.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8739216 [details] [diff] [review]
remove the unneeded assert and associated test annotations

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

Sorry for the noise in mochitest.ini, I did a little housekeeping while I was in there.

::: dom/plugins/test/mochitest/test_GCrace.html
@@ -17,5 @@
>      function start() {
> -      if (navigator.platform.startsWith("Win")) {
> -        SimpleTest.expectAssertions(0, 350);
> -      }
> -

I'm particularly proud of this one. The joys of giving a test a 0-350 allowable range. We're sure to notice when it no longer has issues!

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/09fc4fe79ed0

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/09fc4fe79ed0