Closed Bug 1262050 Opened 4 years ago Closed 3 years ago

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

Categories

(Core :: Plug-ins, defect)

Unspecified
Windows
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
e10s + ---
firefox48 --- fixed

People

(Reporter: RyanVM, Assigned: RyanVM)

References

(Blocks 1 open bug)

Details

(Keywords: assertion)

Attachments

(1 file)

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?
Flags: needinfo?(jmathies)
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
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.
Flags: needinfo?(jmathies) → needinfo?(twalker)
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.
Sure, go ahead, I'll follow along for the learning.
Flags: needinfo?(twalker)
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.
Try says this is good with and without e10s enabled.
Assignee: nobody → ryanvm
Status: NEW → ASSIGNED
Attachment #8739216 - Flags: review?(jmathies)
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!
Attachment #8739216 - Flags: review?(jmathies) → review+
Keywords: leave-open
Target Milestone: --- → mozilla48
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.