openSUSE patches and bugs to review

[David A Benjamin]

Hi Stanislav,

On Tue, 12 Jul 2011, Stanislav Brabec wrote:
> Thanks for taking nspluginwrapper development. It is really a very
> sophisticated code and only few people are able to understand it.

Hehe. Well, it's been rather entertaining working on this.

> As the new project is hosted by Redhat, I guess that all Fedora patches
> were reviewed and incorporated to 1.4.4. That is why I would like to
> browse only those SUSE specific or my own. All patches were intended for
> version 1.3.0. I can port them if they don't fit to 1.4.4.

Actually, it's currently hosted by a combination of Red Hat, GitHub, and 
scripts.mit.edu and mostly worked on by a random college student who at 
one point interned on Chrome, has no Red Hat affiliation, and runs Ubuntu 
on his laptop (though I do have a Fedora box elsewhere). I don't think 
it's fair to claim anyone's getting special treatment here. :-)

I thought I looked through SUSE patches when I first started this effort, 
but I could well have missed them. There were a lot of patches across many 
distros to go through, and I'm sure I missed plenty.

> Problem: Filling disc by not useful reports.
>
> Patches:
>
> nspluginwrapper-crash-silencer.patch: It is probably useful to disable
> all known ways of collecting of core files, or do at least for
> proprietary plugins. Disabling of bug-buddy makes sense for all
> plugins - plugin restart logic kills bug buddy earlier than bug-buddy is
> able to process anything.

Flash may be less crashy than everyone thinks. I'm sure many crashes are 
its fault, but nspluginwrapper is really buggy too, and you often can't 
tell from the stack trace. When I first started working on this, every 
crash I diagnosed was from one of nspluginwrapper's many race conditions. 
I have actually yet to conclusively pin any crash on the Flash plugin.

I hope that 1.4 is sufficiently more stable that you won't hit crashes as 
often, and so core dumps maybe be okay. (Hey, they may even help fix 
bugs.) If the plugin restart logic causes bug-buddy to be non-functional, 
that's reasonable to disable. (Although I'd like to get rid of that logic 
for browsers with out-of-process plugin support.)

> nspluginwrapper-no-debug.patch: It makes only a little sense to keep
> DEBUG half-enabled in production builds. However, if all these debug
> issues will be fixed, reporting remaining of them may make sense
> (version 1.3.0 often filled the whole partition by .xsession-errors and
> core files, with 1.4.4 I don't see any record in the .xsession-errors,
> only flash crashes sometimes).

Please please please do not release a build of nspluginwrapper with that 
patch. If you already have, please revert it.

Removing that setting is actively harmful to resolving bugs. It does 
nothing by default, but when NPW_DEBUG is set, you get a trace of the 
plugin. It is nearly impossible to diagnose a bug without it. And, sadly, 
I cannot claim that even with my work, nspluginwrapper is bug-free. :-)

(It was "half-enabled" in the original tarballs as well. I imagine the 
reason is to avoid cluttering the binary and performance. Also they make 
the trace very noisy. I may revisit this and enable them in rpc.c if it 
turns out to not slow things down. Would need some way to selectively 
enable and disable pieces of debug though.)

> Problem: Incorrect use of curl
>
> nspluginwrapper-curl-threads.patch: nspluginwrapper is an apparent
> candidate of threading triggered curl crashes. However I seen no such
> crash yet, applications like tangogps, viking and others crash exactly
> for that reason. That is why I think the patch should be applied.
> For more see:
> https://sourceforge.net/tracker/?func=detail&atid=570954&aid=3206628&group_id=83870

That code's actually part of nspluginplayer, which may be why you haven't 
seen any crashes. (It shares no code with the wrapper.)  But thank you for 
the patch. Flash spawns threads, so this is definitely worthwhile in the 
player. Applied.

> Problem: native windows implementation
>
> Patch:
>
> nspluginwrapper-native-windows.patch: Differs from the actual
> implementation of GDK_NATIVE_WINDOWS (apply for Adobe Flash only). I
> guess that Adobe Acrobat (and possibly everything else done by Adobe)
> needs the work-around as well, and probably some Java plugins as well. I
> experienced focus problems in PDF viewer search that are may be related
> to that.

Hrm. So I took the one that applied to fewer things to minimize the ways 
nspluginwrapper isn't a dumb proxy. This is really something your browser 
should set (Both Firefox and Chrome do, not sure about WebKitGTK. 
Honestly, I'd like to remove the logic for Flash too). Are you sure your 
PDF viewer issue is related? nppdf.so doesn't even link to GTK. It uses Xt 
routines. (It actually shells out to acroread and fails to run if acroread 
is not in your PATH.)

Java it's possible would be affected, but I'm not aware of any issues. If 
it's necessary, I'm make the setting more liberal. But, again, I'd prefer 
your browser do it instead. It'd match Firefox and Chrome and the browser 
needs this when running the plugin directly anyway. I don't want to get 
into the business of working around browser/plugin bugs that need 
workarounds elsewhere anyway. NPAPI is inconsistent enough as it is.

> Problem: Reaching assert() in the nspluginwrapper code kills the browser.
>
> No patches in openSUSE.

Yeah, that's somewhat unavoidable. If the wrapper process dies, the 
wrapper process dies. You should ask your browser to implement 
out-of-process plugins so that the rest of it doesn't die. 
(nspluginwrapper, for various silly reasons, can't implement it well 
anyway. This is why I do not generally recommend it when your only goal is 
out-of-process plugins. Firefox and Chrome will always be able to do an 
infinitely better job. Arguably the 32-bit/64-bit translation is the 
browser's job too, but that's less widely implemented in today's 
browsers.)

One could almost argue the opposite: even if the /viewer/ crashes, the 
wrapper should crash in response. This would be a huge win for Firefox and 
Chrome users. Unfortunately, it will be bad for many others. Currently 
nspluginwrapper tries to handle this case, but doesn't (and really cannot) 
do a good job. If the wrapper crashes too, Firefox and Chrome can find out 
and display nice "This plugin has crashed" UI and offer to restart it and 
whatnot.

(In fact, the one change I made that is by far most responsible for the 
increased stability of 1.4 was to bind the two halves' event loops 
together so that a plugin hang always results a wrapper hang. NPAPI is 
very synchronous, so any attempts to avoid this in nspluginwrapper are 
doomed cause nasty race conditions.)

> Some of bug reports known to me:
> https://bugs.launchpad.net/ubuntu/+source/nspluginwrapper/+bug/544864
> https://bugzilla.gnome.org/show_bug.cgi?id=628612
> I don't know how to check whether this problem is already fixed i 1.4.4.

I came across that launchpad report actually. Unfortunately, without a 
trace from NPW_DEBUG, I cannot reproduce or debug it, and the stack trace 
isn't very enlightening. I did end up redesigning the NPObject bridge 
though, so it's not inconceivable that bug has been fixed. (Hopefully 
without introducing new bugs in its stead.)


David