1022042 - compareVariants in the test plugin leaks |identifiers|

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Andrew McCreight [:mccr8]]

Attachment: leaking stacks

The NPVariantType_Object case calls into NPN_Enumerate, which allocates a buffer for identifiers, but the test plugin fails to free it.

Comment 1

[Georg Fritzsche [:gfritzsche]]

Andrew, can you add which test this happens in?

Comment 2

[Georg Fritzsche [:gfritzsche]]

The affected code is here:
http://hg.mozilla.org/mozilla-central/annotate/9dc0ffca10f4/dom/plugins/test/testplugin/nptest.cpp#l2217

The note in the documentation mentions what cleanup is expected:
https://developer.mozilla.org/en-US/docs/NPN_Enumerate

Comment 3

[Andrew McCreight [:mccr8]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #1)
> Andrew, can you add which test this happens in?

I'm not sure offhand, but I can bisect it at some point.

Comment 4

[Anmol Agarwal]

I want to solve this bug.It will be my first bug.Please help me.

Comment 5

[Georg Fritzsche [:gfritzsche]]

Hi Anmol,

comment 2 shows the section of the code that is affected and links to the documentation.
If you don't have a working Firefox build yet, see here: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions

Let me know if there are more specific questions (and feel free to drop me an e-mail or talk on IRC if you prefer).

Comment 6

[Andrew McCreight [:mccr8]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #1)
> Andrew, can you add which test this happens in?

This happens in some subtest of Mochitest-other that looks like it just runs things in /dom/plugins/test/mochitest/?  I'm not sure what that is.

Comment 7

[James King]

Attachment: test-plugin-leaks-fix.patch

Is this what you're looking for? I haven't tested as I'm not familiar yet with how to go about that part.

Were the leaks detected from the valgrind tests? Or do the mochitests catch them too? I'm a little confused on that part.

Comment 8

[Andrew McCreight [:mccr8]]

> Were the leaks detected from the valgrind tests? Or do the mochitests catch them too? I'm a little confused on that part.

This was detected by running Mochitests with LeakSanitizer, which is a special mode of our AddressSanitizer builds.  It hasn't landed yet.  (Bug 988041)

Comment 9

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8441562 [details] [diff] [review]
test-plugin-leaks-fix.patch

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

This looks good, thanks James!
It would be great if you can test this. We were missing information on it in this bug, let me fix this.

You first need to have an ASAN build:
https://developer.mozilla.org/en-US/docs/Mozilla/Testing/Firefox_and_Address_Sanitizer#Manual_Build

Then you should check out the LSAN section Andrew added:
https://developer.mozilla.org/en-US/docs/Mozilla/Testing/Firefox_and_Address_Sanitizer#LeakSanitizer

Now, without your patch applied, one of these test-runs should trigger the issue Andrew reported:
> mach mochitest-plain dom/plugins
> mach mochitest-chrome dom/plugins

Running again with your patch applied, the problem should be resolved.

Let me know if there are any issues and i'll check it out.

Comment 10

[Andrew McCreight [:mccr8]]

I can also just test this patch myself.  Unless you are on Linux, ASAN builds are either hard (OSX) or impossible (Windows).

Comment 11

[James King]

I can try it in a VM. Thanks for the links.

Comment 12

[Andrew McCreight [:mccr8]]

It looks like the leak is actually happening with mochitest-plugins, which seems to be a version of dom/plugins that runs with in-process plugins.  I wasn't able to figure out how to run it locally in the right way.

Comment 13

[Georg Fritzsche [:gfritzsche]]

Andrew, maybe you can just do a push with the fix and see if it is fine now?

Alternatively, i think we could just override it locally by adding:
> user_pref("dom.ipc.plugins.enabled.i386", false);
> user_pref("dom.ipc.plugins.enabled.x86_64", false);
... to the testing profiles:
http://mxr.mozilla.org/mozilla-central/source/testing/profiles/prefs_general.js

Comment 14

[Andrew McCreight [:mccr8]]

Yeah, I can just check it at some point in the next day or two.

Comment 15

[Andrew McCreight [:mccr8]]

Ok, I finally got around to doing a test run for this.  We'll see how it goes:
  https://tbpl.mozilla.org/?tree=Try&rev=5d2aba8e2226
Since LSan has now landed, if this works, please incorporate the removal of the suppression for this so we can catch any regressions in the future.  That would just be this little patch:
  https://hg.mozilla.org/try/rev/ba4c8c806393

Comment 16

[Andrew McCreight [:mccr8]]

Yep, it looks good to me, this patch fixed the leak.

Comment 17

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8441562 [details] [diff] [review]
test-plugin-leaks-fix.patch

Thanks for checking it out Andrew.

James, sorry for the turn-around time here - this should be landing now after Benjamin signed it off.
Could you upload a patch with a suitable commit message?
There are some details here: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 18

[James King]

Attachment: fix-compareVariants-leak.patch

Comment 19

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8445165 [details] [diff] [review]
fix-compareVariants-leak.patch

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

Thanks, that looks good. I'm no actual peer for plugins code so just feedback+ from me.

Comment 20

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/86d027848ae0
https://hg.mozilla.org/integration/mozilla-inbound/rev/d358bea25531

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/86d027848ae0
https://hg.mozilla.org/mozilla-central/rev/d358bea25531