386676 - Regression tests needed for plugins

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Mike Schroepfer]

Per discussion on email we need to get some better automated regression coverage of plug-ins:


> Hey Martijn/Johnny,
> 
> There have been a number of tricky/nasty crashers related to plug-ins (eg.
> 347743) - is there a way we can get better automated testing of plug-ins?
> Say writing a mock plugin that stresses out the api?


That could certainly be done to some degree. I.e. we could emulate WMP going back to the win32 message queue when shutting it down, and we could intentionally screw up on reference counting some NPRuntime objects in a plugin etc. And I bet there's far more than that we could do if someone sat down and went through old bugs etc.

Comment 1

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

http://mxr.mozilla.org/mozilla/source/modules/plugin/tools/tester/ has a test plugin that was originally designed to exercise NPAPI.  It broke in 2002, and wasn't fixed, though the patch in the bug might still apply, since that code hasn't changed a lot: https://bugzilla.mozilla.org/show_bug.cgi?id=163888 .  We'd probably also need to fix https://bugzilla.mozilla.org/show_bug.cgi?id=105959 to make it run on other platforms.

Before discovering those in my MXR digging, I also spent about an hour learning how NPAPI and scriptability worked, and hacked a basic test plugin together from the test code for scriptable plugins (http://mxr.mozilla.org/mozilla/source/modules/plugin/samples/npruntime/).  The hardest part so far, honestly, was figuring out how to put the plugin in the mochitest profile rather than building it everywhere by default, but I believe that bug 378422 will let me use an extension dir copied into the mochiprofile and win.  The plugin doesn't do much now, mainly because I don't know much about the plugin APIs and what's useful to test, but I'll hack on it some more on the plane tomorrow and see what I can come up with.  Someone who actually knows the plugin code could probably revive nptester or repurpose samples/npruntime to utility much more quickly, and I would not be offended if they did so.

Comment 2

[Josh Aas]

Attachment: fix v0.7

This works on Mac OS X and I believe on Linux, though the latter I have not tested myself since I made some other changes. I still have to do another memory leak audit, test on Linux, and make sure the plugin at least builds on Windows. Also, this patch includes some patches that I have broken off into other bugs but have not yet landed.

The one small mochitest in this patch gives us a lot of coverage we haven't had before. It tests much of the loading process for plugins plus basic npruntime scriptability into and out of plugins. There are obviously a lot of tests we could build on top of this, my goal for now is to make a system that makes that easy to do.

Comment 3

[Josh Aas]

Attachment: fix v0.8

Comment 4

[Johnny Stenback (:jst)]

Josh, I forgot to mention yesterday when I was helping you debug the problem of functions not being exposed to JS by this plugin that I saw a shutdown crash that's most likely related to this plugin (setWindow related IIRC). I think it was 100% re-produceable, but I can't say for sure and I was focusing on the other problem and didn't get to looking into the crash. So that needs to be looked into before this can go live.

Comment 5

[Josh Aas]

Attachment: fix v1.0

This builds on Mac OS X, Linux, and Windows. Tests are enabled and they pass on Mac OS X and Linux.

Some basic work that remains is documented in the patch:
- fix bug that prevents drawing on Mac OS X
- write drawing code for Linux
- finish porting the plugin to Windows, such that tests pass, and enable tests on Windows, right now the plugin merely compiles on Windows

Note that the plugin loads and communicates via npruntime even if it can't draw.

I don't think any of these remaining work items need to hold up review and landing.

This patch has a lot of fixes since the last one I posted, including a fix for the crash jst reported.

Comment 6

[Josh Aas]

Attachment: fix v1.1

Some more cleanup, draws on Mac OS X.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 349771 [details] [diff] [review]
fix v1.1

This looks fine to me, though I have a couple of quibbles:

- * Copyright © 2004, Apple Computer, Inc. and The Mozilla Foundation.
+ * Copyright � 2004, Apple Computer, Inc. and The Mozilla Foundation.

You somehow corrupted the copyright character at the top of npruntime.h.

+void
+scriptableDeallocate(NPObject* npobj)
+{
+  NPN_MemFree((TestNPObject*)npobj);
+}

You don't need to cast npobj to TestNPObject* -- NPN_MemFree() expects a
void*.

+  // draw the UA string using ATSUI [in drawPlugin(NPP instance)]

Too bad nptest.cpp isn't an *.mm file, or your task would have been much
simpler -- you could just have used [NSString drawAtPoint:withAttributes:] (as
Apple's WebKit test plugin for the CoreGraphics drawing model does).

By the way, I ran the test_npruntime.xul chrome test and it passed.

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #7)
> (From update of attachment 349771 [details] [diff] [review])
> This looks fine to me, though I have a couple of quibbles:
> 
> - * Copyright © 2004, Apple Computer, Inc. and The Mozilla Foundation.
> + * Copyright � 2004, Apple Computer, Inc. and The Mozilla Foundation.
> 
> You somehow corrupted the copyright character at the top of npruntime.h.
> 
> +void
> +scriptableDeallocate(NPObject* npobj)
> +{
> +  NPN_MemFree((TestNPObject*)npobj);
> +}
> 
> You don't need to cast npobj to TestNPObject* -- NPN_MemFree() expects a
> void*.

Maybe struct TestNPObject should derive from NPObject instead of embedding it as the first member? Then there'd be less doubt about the address not needing adjustment via a (static) cast.

/be

Comment 9

[Brendan Eich [:brendan]]

Of course it's easy to see that the npobject is first, but if this code is meant as a template of sorts, or could be imitated and extended, it might be better to future-proof a bit.

/be

Comment 10

[Josh Aas]

Attachment: fix v1.0

Comment 11

[Josh Aas]

Sorry I mis-versionted this last patch, should say fix v1.2.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+// This is a windowless plugin on all platforms, Mac OS X assumes so and won't
+// draw if we say so explicitly. See bug 466495.
+#ifndef XP_MACOSX
+  NPN_SetValue(instance, NPPVpluginWindowBool, NULL);
+#endif

We've fixed this on trunk so can you make this unconditional now?

Is putting all the per-platform code into one file with #ifdefs really the way to go? Could we break it up into per-platform helper functions and link in the right helper file based on the platform?

Comment 13

[Josh Aas]

Attachment: fix v1.1

remove the windowless ifdef and update to tip

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  pFuncs->version = 11;
+  pFuncs->size = sizeof(*pFuncs);
+  pFuncs->newp = NPP_New;
+  pFuncs->destroy = NPP_Destroy;
+  pFuncs->setwindow = NPP_SetWindow;
+  pFuncs->newstream = NPP_NewStream;
+  pFuncs->destroystream = NPP_DestroyStream;
+  pFuncs->asfile = NPP_StreamAsFile;
+  pFuncs->writeready = NPP_WriteReady;
+  pFuncs->write = NPP_Write;
+  pFuncs->print = NPP_Print;
+  pFuncs->event = NPP_HandleEvent;
+  pFuncs->urlnotify = NPP_URLNotify;
+  pFuncs->getvalue = NPP_GetValue;
+  pFuncs->setvalue = NPP_SetValue;

This block can be shared in a helper function instead of writing it out twice.

Maybe the initialization stuff can stay here #ifdefed. But moving drawPlugin and NPP_HandleEvent out to a separate, per-platform file would get rid of most of the ugliness, I think, and be very easy. Please do that and then we'll see how the patch looks.

Comment 16

[Josh Aas]

Attachment: fix v1.2

General fixes, BSD licensing, enable support for Windows, basic system for organizing tests, better first test, platform-specific files.

Includes a WebKit compatibility fix that wasn't in earlier patches.

-    const NPUTF8 *utf8characters;
-    uint32_t utf8length;
+    const NPUTF8 *UTF8Characters;
+    uint32_t UTF8Length;

Use the same names as WebKit does for these struct members.

Word on the street is that the test does not pass on Windows, which is odd since none of the test code itself should be platform-specific.

Comment 17

[Josh Aas]

Attachment: fix v1.3

Windows fixes from Jim Mathies.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+void fillPluginFunctionTable(NPPluginFuncs* pFuncs)

static void

+void drawPlugin(NPWindow window, char* browserUAString);

How about pluginDraw so that as we add functions, they can be named consistently?

+#ifdef XP_MACOSX
+  NPBool supportsCoreGraphics = false;
+  if (NPN_GetValue(instance, NPNVsupportsCoreGraphicsBool, &supportsCoreGraphics) == NPERR_NO_ERROR && supportsCoreGraphics) {
+    NPN_SetValue(instance, NPPVpluginDrawingModel, (void*)NPDrawingModelCoreGraphics);
+  } else {
+    printf("CoreGraphics drawing model not supported, can't create a plugin instance.\n");
+    return NPERR_INCOMPATIBLE_VERSION_ERROR;
+  }
+#endif

Let's abstract this out into a platform-specific helper function (which does nothing on Windows/GTK) ... say pluginInit? Also, shouldn't there be some "plugin object" to store plugin state in, that's passed to these functions?

NPP_HandleEvent should also be moved out to be defined per-platform, or have its guts moved to a helper function ... say pluginHandleEvent?

Comment 19

[Josh Aas]

Attachment: fix v1.4

Comment 20

[Josh Aas]

Attachment: fix v1.5

Comment 21

[Josh Aas]

pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/2776db2defa2

Comment 22

[Josh Aas]

backed out due to leaks

Comment 23

[Josh Aas]

I re-landed the NPAPI header parts of the patch.

Comment 24

[Josh Aas]

Checkin URL for the NPAPI header parts is:

http://hg.mozilla.org/mozilla-central/rev/dc8bfdabdd7f

Comment 25

[Josh Aas]

Attachment: fix v1.6

Update to mozilla-central tip.

I'm not able to reproduce any leaks locally, I'm not sure it was really this patch leaking earlier today.

Comment 26

[Benjamin Smedberg]

This bug caused the following:
NEW WARNING:
  modules/plugin/base/public/npfunctions.h:61: C++ style comments are not allowed in ISO C90 - Blamed on Josh Aas <[e-mail]> - http://hg.mozilla.org/mozilla-central/annotate/dc8bfdabdd7fd64a9270b2d127efff497d1bbb57/modules/plugin/base/public/npfunctions.h#l61

Please use C-style comments in NPAPI headers.

Comment 27

[Josh Aas]

That was fixed immediately following the original checkin last night.

Comment 28

[Josh Aas]

Attachment: fix v1.7

This fixes a significant leak in the plugin, but unfortunately not the leak that caused tinderboxes to show leaks.

I did narrow the leak down to a particular non-chrome test, one for bug 391728:

content/base/test/test_bug391728.html

All you have to do to see the leak is run that test.

Comment 29

[Josh Aas]

Attachment: fix v1.8

This includes a couple disabled tests and a build fix for some environments. Unfortunately there are still more leaks exposed by this plugin.

Comment 30

[Josh Aas]

All of the leaks I've found so far result from the line "plugin.disabled=true" - disabling a plugin that has a live instance running leaks. When I have more info and I figure out what is going on with the rest of these leaks I'll file a bug.

Comment 31

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Fixed at least one leak here, see bug 472439.

Comment 32

[Josh Aas]

pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/7b1292782cd3

Comment 33

[Mark Banner (:standard8)]

This has caused bustage on the Thunderbird trunk:

http://tinderbox.mozilla.org/showlog.cgi?log=Thunderbird/1231834301.1231834432.5161.gz

/buildbot/linux-comm-central-check/build/mozilla/modules/plugin/test/testplugin/nptest_utils.cpp:61: error: ‘assert’ was not declared in this scope
/buildbot/linux-comm-central-check/build/mozilla/modules/plugin/test/testplugin/nptest_utils.cpp:71: error: ‘assert’ was not declared in this scope
/buildbot/linux-comm-central-check/build/mozilla/modules/plugin/test/testplugin/nptest_utils.cpp:72: error: ‘int32’ was not declared in this scope
/buildbot/linux-comm-central-check/build/mozilla/modules/plugin/test/testplugin/nptest_utils.cpp:72: error: expected `;' before ‘integer’
/buildbot/linux-comm-central-check/build/mozilla/modules/plugin/test/testplugin/nptest_utils.cpp:73: error: ‘integer’ was not declared in this scope
/buildbot/linux-comm-central-check/build/mozilla/modules/plugin/test/testplugin/nptest_utils.cpp:79: error: ‘assert’ was not declared in this scope
/buildbot/linux-comm-central-check/build/mozilla/modules/plugin/test/testplugin/nptest_utils.cpp:82: error: ‘int32’ was not declared in this scope
/buildbot/linux-comm-central-check/build/mozilla/modules/plugin/test/testplugin/nptest_utils.cpp:82: error: expected `;' before ‘integer’
/buildbot/linux-comm-central-check/build/mozilla/modules/plugin/test/testplugin/nptest_utils.cpp:83: error: ‘integer’ was not declared in this scope
gmake[8]: *** [nptest_utils.o] Error 1
gmake[7]: *** [libs] Error 2
gmake[6]: *** [libs] Error 2
gmake[5]: *** [libs_tier_gecko] Error 2
gmake[4]: *** [tier_gecko] Error 2
gmake[3]: *** [default] Error 2
make[2]: *** [default] Error 2
make[1]: *** [build] Error 2


It looks like Firefox is pulling in different headers for some reason (I checked the deps files), its either building something extra or because it is an --enable-libxul build.

The solution appears to be to include assert.h and protypes.h (or jsotypes.h), however *otypes.h are both files that are meant to be obsolete. That would imply PRInt32 should be used in nptest_utils.cpp rather than int32, however as I don't know the code, I haven't submitted a bustage fix.

Comment 34

[Mark Banner (:standard8)]

On Thunderbird's Windows leak/bloat tinderbox (debug build) I'm getting:

d:/buildbot/comm-central-trunk-bloat-win32/build/mozilla/modules/plugin/test/testplugin/nptest.cpp(230) : error C3861: 'printf': identifier not found

Doesn't seem to happen on a non-debug build.

Comment 35

[Josh Aas]

I checked in a fix, included "assert.h" and changed int32 to int32_t. Thanks for the heads up and let me know if there is anything else.

Comment 36

[Mark Banner (:standard8)]

(In reply to comment #35)
> I checked in a fix, included "assert.h" and changed int32 to int32_t. Thanks
> for the heads up and let me know if there is anything else.

Thanks Josh, the only issue I think we have now is on Windows:

d:/buildbot/comm-central-trunk-bloat-win32/build/mozilla/modules/plugin/test/testplugin/nptest.cpp(230) : error C3861: 'printf': identifier not found

Comment 37

[Mark Banner (:standard8)]

(In reply to comment #36)
> (In reply to comment #35)
> > I checked in a fix, included "assert.h" and changed int32 to int32_t. Thanks
> > for the heads up and let me know if there is anything else.
> 
> Thanks Josh, the only issue I think we have now is on Windows:
> 
> d:/buildbot/comm-central-trunk-bloat-win32/build/mozilla/modules/plugin/test/testplugin/nptest.cpp(230)
> : error C3861: 'printf': identifier not found

I've just been able to build/test it so I've checked in an additional bustage fix to add stdio.h:

http://hg.mozilla.org/mozilla-central/rev/32165e24519a

Comment 38

[Josh Aas]

Thanks!

Comment 39

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Mark, so it does work for you now? Do you have a chance to verify it?

This is trunk only and/or too late for 1.9.1?

Comment 40

[(not currently active) Ted Mielczarek]

It's just testing changes, we could take it on 1.9.1. (And it shouldn't need approval either.)

However, the leak fixes that made it possible to land this might need approval to land on 1.9.1.

Comment 41

[Mark Banner (:standard8)]

The fixes did work as the Thunderbird trunk tinderboxes verify: http://tinderbox.mozilla.org/Thunderbird/

Comment 43

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Josh, due to we don't have approval and it's not a blocking bug the tests will be checked into 1.9.1 branch after b3? Or what are your plans?

No more reports lately and at least the tests for private browsing mode are working perfectly. Marking as verified.

Comment 44

[Josh Aas]

I don't plan to check this stuff in on the 1.9.1 branch. We'd have to pull in a bunch of collateral patches and I'm not sure it is worth it. Plugin patches for 1.9.1 will get checked in on trunk first so the tests are still protecting that branch indirectly.