Which some plugins, e.g. java (cf. bug 83376 comment 154) don't like. This is related to bug 83376, but I'd like to discuss this separately since that bug has become too cluttered. I've run into this bug myself with the Epiphany RPMs for Mandriva2006 linux distribution (has been fixed by them by shortening their UA string additions). This is esp. concerning since adding extra UA portions is now very easy with the eneral.useragent.extra.* prefs from bug 274928; the current limit is 128 characters [http://lxr.mozilla.org/seamonkey/source/modules/plugin/base/src/nsPluginHostImpl.cpp#2778] which is already easily attained with one or two extras. I therefore propose to eliminate the 128 characters limit from nsPluginHostImpl::mUserAgent. Now I can see a few problems arise: - currently user agent is stored in a static char[128]. While NPN_UserAgent returns a const char*, some plugins might have relied on this string being static (i.e. outliving themselves), and stored it internally - some plugins may rely on the 128 characters limit. Worse, two of mozilla's sample plugins actually do this: http://lxr.mozilla.org/seamonkey/source/modules/plugin/samples/4x-scriptable/plugin.h#59 + http://lxr.mozilla.org/seamonkey/source/modules/plugin/samples/4x-scriptable/plugin.cpp#63 and http://lxr.mozilla.org/seamonkey/source/modules/plugin/tools/sdk/samples/scriptable/mac/plugin.h#61 + http://lxr.mozilla.org/seamonkey/source/modules/plugin/tools/sdk/samples/scriptable/mac/plugin.cpp#299

For what it's worth, it appears that this limit was imposed as an implementation detail after npapi had been around for a while. See bug 58095 for the patch that introduced it. It's also not advertised in the api itself that there's a limit, so it's unlikely that plugins will do bad things with >= 128 characters. However, there's an easy way to test. Raise the limit and test as many plugins as possible.

This affects all platforms (I saw it on Windows).

Given the fact that some plugins may assume the UA string returned will be < 128 characters, I think the best solution is to return a truncated UA string. My proposed fix truncates at the right-most space character that is within the length we have to work with.

I think I'd rather just remove the limit and fix our internal retarded samples to not depend on that. I wonder if Safari has a similar limit too? If not, I'd say we just remove the limit and fix the samples. And now would be a good time to do that, get it out there and see if plugins start crashing left and right or not.

I propose that we land the current patch (assuming it passes reviews) since it will fix the Flash plugin crash. Then we can file a new bug to remove the limit and adjust the sample plugins. Right now, I don't have the resources to take on the work needed to create a patch for the sample plugins.

Comment on attachment 324046 [details] [diff] [review] proposed fix (return truncated UA string) Fair enough.

Thank you! Seeking approval to land for Firefox 2.0.0.x and 3.0.x since this fixes plugin crashes.

blocking1.8.0.15 is for the (officially) discontinued Firefox 1.5.0.x releases. You want blocking1.8.1.17 for the 2.0.0.x branch.

Thanks, I must have misread the tooltip. Requesting approval to land for Firefox 2.0.0.x and 3.0.x since this fixes plugin crashes.

(In reply to comment #10) > Requesting approval to land for Firefox 2.0.0.x and 3.0.x since this fixes > plugin crashes. You need to set the approval? flags on the patch. I went ahead and did that for you.

Can we get an automated test for this? We also need this landed on mozilla-central before approving for either branch. (I know, I know, it's only been a few hours since the bug was last touched...)

Comment on attachment 324046 [details] [diff] [review] proposed fix (return truncated UA string) Please re-request approval after this lands and bakes on mozilla-central.

Sorry I haven't been able to land yet; waiting for my Mercurial account...

This bug doesn't fit the 1.8 "blocking" criteria (not security, not recent regression, not topcrash), but if this gets into the trunk and has a testcase QA can use to verify we could approve a branch patch.

Kathy, I just committed this for you, so this is fixed now. Thanks for the fix! http://hg.mozilla.org/mozilla-central/index.cgi/rev/3884f9437753

We won't block on this for 1.9.0.2 either, but will gladly take a baked patch with a test.

Is this bug only fixed on trunk? It still crashes PDF plugin on branch (Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.0.3pre) Gecko/2008090306 GranParadiso/3.0.3pre) as I reported in bug 451944.

Currently, this bug is fixed only on the trunk (FF 3.1.*). I would love to check in the patch to the 3.0 tree but it lacks automated tests. This patch fixes crashes within plugins but I'm not sure if or how those crashes would be reflected in topcrash data since the stack may be different depending on which plugin crashes.

Comment on attachment 324046 [details] [diff] [review] proposed fix (return truncated UA string) Too late for 1.9.0.2, and still no test so no point in sticking on the 1.9.0.3 list yet. Doesn't mozilla-central also require tests?

Comment on attachment 324046 [details] [diff] [review] proposed fix (return truncated UA string) dveditz: this isn't reasonble, we are going to help out roboform but not acrobat plugin? this patch has baked.

timeless: This bug really needs a test before we're willing to take it on branch. An automated test would be best, but a manual test that QA can run would likely be sufficient for us to look at this.

I'm sorry; I thought I had included this in the bug. Here is a manual test case for Windows XP (Firefox 3.0.*): 1) Make sure you have Adobe Reader 8 installed (I have 8.1.2) and that it is configured so that the plugin is active in Firefox (show PDF files in browser). 2) Use about:config to add this new string preference: name: general.useragent.extra.bug328778 value: 1234567890123456789012345678901234567890 (The value needs to be long enough so the UA string is more than 128 chars.) 3) Load any PDF file in Firefox, such as http://www.mozilla.org/foundation/donate_form.pdf 4) Firefox will crash.

Comment on attachment 324046 [details] [diff] [review] proposed fix (return truncated UA string) Couldn't this whole chunk have been a single call PL_strncpyz(resultString, uaString.get(), NS_RETURN_UASTRING_SIZE); ? I guess that wouldn't truncate on a space, not as nice but do we really care?

personally, i'd rather keep braces if there's an else clause that has one. the code could possibly be simpler w/ strrchr, but i like the space condition.

Comment on attachment 324046 [details] [diff] [review] proposed fix (return truncated UA string) Approved for 1.9.0.4, a=dveditz for release-drivers

This patch has now landed in CVS (for Firefox 3.0.x). modules/plugin/base/src/nsPluginHostImpl.cpp new revision: 1.609; previous revision: 1.608

Verified for 1.9.0.4 using the testcase in comment 26 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.4pre) Gecko/2008102405 GranParadiso/3.0.4pre.