Regression tests needed for plugins

VERIFIED FIXED in mozilla1.9.2a1

Status

()

defect
VERIFIED FIXED
12 years ago
11 years ago

People

(Reporter: mtschrep, Assigned: jaas)

Tracking

(Blocks 1 bug)

Trunk
mozilla1.9.2a1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 -
wanted1.9 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 12 obsolete attachments)

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.
Flags: blocking1.9?
Flags: blocking1.9? → blocking1.9-
Whiteboard: [wanted-1.9]
Flags: wanted1.9+
Whiteboard: [wanted-1.9]
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.
Posted patch fix v0.7 (obsolete) — Splinter Review
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.
Assignee: nobody → joshmoz
Posted patch fix v0.8 (obsolete) — Splinter Review
Attachment #349463 - Attachment is obsolete: true
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.
Posted patch fix v1.0 (obsolete) — Splinter Review
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.
Attachment #349488 - Attachment is obsolete: true
Attachment #349706 - Flags: review?(smichaud)
Posted patch fix v1.1 (obsolete) — Splinter Review
Some more cleanup, draws on Mac OS X.
Attachment #349706 - Attachment is obsolete: true
Attachment #349771 - Flags: review?(smichaud)
Attachment #349706 - Flags: review?(smichaud)
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.
Attachment #349771 - Flags: review?(smichaud) → review+
(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
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
Posted patch fix v1.0 (obsolete) — Splinter Review
Attachment #349771 - Attachment is obsolete: true
Attachment #351394 - Flags: superreview?(jst)
Sorry I mis-versionted this last patch, should say fix v1.2.
Attachment #351394 - Flags: superreview?(jst) → superreview?(roc)
+// 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?
Posted patch fix v1.1 (obsolete) — Splinter Review
remove the windowless ifdef and update to tip
Attachment #351394 - Attachment is obsolete: true
Attachment #352189 - Flags: superreview?(roc)
Attachment #351394 - Flags: superreview?(roc)
+  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.
Duplicate of this bug: test-plugin
Posted patch fix v1.2 (obsolete) — Splinter Review
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.
Attachment #352189 - Attachment is obsolete: true
Attachment #352189 - Flags: superreview?(roc)
Posted patch fix v1.3 (obsolete) — Splinter Review
Windows fixes from Jim Mathies.
Attachment #352773 - Attachment is obsolete: true
Attachment #353097 - Flags: superreview?(roc)
+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?
Posted patch fix v1.4 (obsolete) — Splinter Review
Attachment #353097 - Attachment is obsolete: true
Attachment #353156 - Flags: superreview?(roc)
Attachment #353097 - Flags: superreview?(roc)
Posted patch fix v1.5 (obsolete) — Splinter Review
Attachment #353156 - Attachment is obsolete: true
Attachment #353162 - Flags: superreview?(roc)
Attachment #353156 - Flags: superreview?(roc)
Attachment #353162 - Flags: superreview?(roc) → superreview+
pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/2776db2defa2
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
backed out due to leaks
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I re-landed the NPAPI header parts of the patch.
Checkin URL for the NPAPI header parts is:

http://hg.mozilla.org/mozilla-central/rev/dc8bfdabdd7f
Posted patch fix v1.6 (obsolete) — Splinter Review
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.
Attachment #353162 - Attachment is obsolete: true
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 <joshmoz@gmail.com> - http://hg.mozilla.org/mozilla-central/annotate/dc8bfdabdd7fd64a9270b2d127efff497d1bbb57/modules/plugin/base/public/npfunctions.h#l61

Please use C-style comments in NPAPI headers.
That was fixed immediately following the original checkin last night.
Posted patch fix v1.7 (obsolete) — Splinter Review
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.
Attachment #353364 - Attachment is obsolete: true
Posted patch fix v1.8Splinter Review
This includes a couple disabled tests and a build fix for some environments. Unfortunately there are still more leaks exposed by this plugin.
Attachment #353638 - Attachment is obsolete: true
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.
Fixed at least one leak here, see bug 472439.
pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/7b1292782cd3
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
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.
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.
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.
(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
(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
Thanks!
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?
Target Milestone: --- → mozilla1.9.2a1
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.
The fixes did work as the Thunderbird trunk tinderboxes verify: http://tinderbox.mozilla.org/Thunderbird/
Depends on: 474503
Duplicate of this bug: 105959
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.
Status: RESOLVED → VERIFIED
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.
You need to log in before you can comment on or make changes to this bug.