406251 - int value conversion problem in NPRuntime, doesn't work with large ints (high bits set)

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect, P2)

Description

[Johnny Stenback (:jst)]

As Kenneth Russell pointed out in person to me, the NPRuntime API layer doesn't properly deal when passing large integer values from a plugin to JS. We fail to see that an integer value doesn't fit in an integer jsval... Patch coming up.

Comment 1

[Johnny Stenback (:jst)]

Attachment: Don't mess up with large ints.

This makes us use JS_NewNumberValue() instead of assuming INT_TO_JSVAL will do the right thing with all ints. This code is identical (except for the automatic cast from int to double) to what we do for double NPVariants.

Comment 2

[Johnny Stenback (:jst)]

Fix checked in. Kenneth, please let me know here if you're still seeing integer conversion problems with NPRuntime. This fix should appear in nightly builds from tomorrow or later.

Comment 3

[Kenneth Russell]

I apologize for the long delay in verifying this bug fix, but unfortunately it is not working correctly. At this point, any negative value passed to INT32_TO_NPVARIANT is coming through with the wrong value -- a large positive value instead of a negative value.

Comment 4

[Johnny Stenback (:jst)]

So, um, this is really lame. I don't know how far back this goes, but as far back as I've been able to find so far (i.e. predates the first checkin of this code into the Mozilla CVS tree, pre firefox 1.0). The plugin futures wiki is having problems atm, so I can't see any more history right now, and I'm not sure how much there ever was in the wiki, but my email archives haven't reviled when this was first introduced yet.

The problem is that NPVariant.intValue is an uint32_t, not an int32_t! So the code in this patch simply casts the unsigned int to a double, and even if the int was originally a negative value, by the time we get into here it's an unsigned int, so it'll be a big positive number instead of the negative number it should be :(

I don't know what the best way to solve this at this point is. Seems like the obvious fix would be to fix the broken type in NPVariant, but that'll come at the risk of potentially breaking some plugins out there when they're recompiled, and even during runtime no matter how we fix this.

Anyone have thoughts either way here?

Comment 5

[Kenneth Russell]

While there's always risk of breakage, since the only difference between uint32_t and int32_t is interpretation of the bit pattern, the impact of changing NPVariant's intValue specification incompatibly would probably be minimal.

Assuming there wasn't a strong reason for making the intValue type unsigned, the fact that nobody saw the error in handling of large integer values (with either of the two high bits set) before the original fix for this bug also implies that there would be very little breakage of existing NPRuntime plugins. Before the original bug fix, certain large integer values placed into NPVariants would be interpreted as negative numbers by Firefox's JavaScript engine.

Comment 6

[Johnny Stenback (:jst)]

Yeah, I do agree. Unless I find evidence somewhere showing that this was done for a reason, or someone can think of a reason why changing this now would be bad, I'm inclined to simply change the definition of NPVariant.intValue in npruntime.h. 

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I tend to agree. It seems really unlikely that someone will be using integers bigger than 2^30.

Comment 8

[Boris Zbarsky [:bzbarsky]]

I think making that a signed int is perfectly reasonable.

Comment 9

[Johnny Stenback (:jst)]

Attachment: Change NPVariant.intType to be int32_t.

I'm still waiting for feedback on this (or lack there of) on the plugin-futures list, I'll probably hold of a couple of days before requesting reviews etc.

Comment 10

[Mike Shaver (:shaver emeritus)]

How do you propose this is tested?

Comment 11

[Johnny Stenback (:jst)]

We have no test infrastructure in place for testing anything related to plugins, the best I can do is either check it in and wait for Kenneth to test, or provide builds from our try servers for him to test. Given the nature of the change, I'd rather just land it, once I'm ready to.

Comment 12

[Kenneth Russell]

I can test the patch with the new Java Plug-In once it lands.

Comment 13

[Johnny Stenback (:jst)]

Thank you Kenneth. In the interest of speeding this up a bit, I did just kick off a try server build. I'll report back once the builds are available and we can hopefully get this closed out even sooner.

Comment 14

[Mike Shaver (:shaver emeritus)]

I'm more worried about regressions against existing plugins.  I'm not asking about automated tests here, just how we plan to check the cases in which Flash or Java or whatever deal in large-int NPVariants.  Manual generation of content would be fine, especially if we file some bugs tracking making them automated.

(I'll stop short of asking for a simple test plugin to be written or automated, because there does not seem to be general agreement that it's important to have such things in place.  But we're fixing a bug that's not new at all, so I don't actually know why it would need to be rushed into production, in the end-game, without test support.  I'd think that the lack of automated coverage would make it _more_ important to do good manual testing, though, with major plugins providing scriptable access.  Am I out to lunch?)

Comment 15

[Kenneth Russell]

Please see my comment #5 above. There was a very longstanding bug in Firefox where very large int NPVariants would be treated incorrectly by the Firefox JavaScript engine, which Johnny fixed with his initial fix for this bug. Nobody ever discovered this until I reported it with the new NPRuntime-based Java Plug-In. Therefore I think it is highly unlikely that any Flash user will see any change in behavior because if they had ever tried to send large int values into the Firefox JS engine they would have encountered this bug by now.

Comment 16

[Johnny Stenback (:jst)]

Shaver, I'm with you that having tests for this, any kind of tests, would be beneficial, but we don't, and we won't before we release either (unless someone with the interest and cycles to do it shows up out of the blue).

The only reason this bug was even found was because of Sun's work on the new NPRuntime enabled Java plugin, primarily through Kenneth. They're by *far* the heaviest users of the NPRuntime API we've seen so far, and that's why these issues are being found now, rather than earlier in our release cycle. The best we can do is to get it out there as soon as possible for more testing.

Comment 17

[Johnny Stenback (:jst)]

Kenneth, builds with the fix are now available here:

https://build.mozilla.org/tryserver-builds/2008-03-11_16:[e-mail]-npvariant-int-type/

Comment 18

[Mike Shaver (:shaver emeritus)]

OK, that's fair.  Kenneth: do you think you could make public an applet and script that pokes at this hard, so that we can stick it in whatever test setup we end up with for Java and plugins?

Comment 19

[Kenneth Russell]

Attachment: Regression test case (run with new NPRuntime-based Java Plug-In)

Fix verified by taking out the workaround from the new NPRuntime-based Java Plug-In and running the attached test case (simply navigate to index.html). Thanks for the prompt fix.

Note that the failure can not be provoked with our publicly visible bits (http://jdk6.dev.java.net/6uNea.html) -- but as soon as the patch lands, I'll take out our workaround for this bug, at which point it can be used as a regression test. (It's almost completely automated aside from a "Test Passed" / "Test Failed" dialog at the end.)

Comment 20

[Johnny Stenback (:jst)]

I've heard only agreement to making this change on the plugin-futures list, so this is ready to go in.

Comment 21

[Johnny Stenback (:jst)]

Fixed.