348710 - Passing an array and its size to an XPCOM method does not work

Status: RESOLVED FIXED

(Core Graveyard :: Java to XPCOM Bridge, defect)

Description

[Vladimir Korenev]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.5) Gecko/20060802 Firefox/1.5.0.5
Build Identifier: 

There are some XPCOM methods that accept an array and its size, e.g. nsIWebBrowserStream::appendToStream(). If the size parameter comes after the array, it is not initialized when the array size is being calculated. This causes invalid data being passed to the method.

Reproducible: Always

Comment 1

[Vladimir Korenev]

Attachment: Patch

Comment 2

[Vladimir Korenev]

Attachment: Test Case

Comment 3

[Martijn Wargers (dead)]

Vladimir, if you want the patch to be reviewed, you need to set the review flags to it, probably [e-mail] should review it (I'm not sure, though).
Thanks for the patch.

Comment 4

[jhp (no longer active)]

Comment on attachment 233754 [details] [diff] [review]
Patch

I am currently taking a look at this and the other bugs opened by Vladimir.

Comment 5

[Vladimir Korenev]

Attachment: The same patch without indentation

This patch does not change code indentation, so it is clearer.

Comment 7

[jhp (no longer active)]

Attachment: patch

The problem was with methods in which the array size parameter came after the array parameter.  This patch fixes it.  Also handles sized strings, and handles an issue with byte arrays.

Comment 8

[jhp (no longer active)]

Checked in to trunk. -> FIXED

Comment 9

[Martijn Wargers (dead)]

There was no review needed for this?
Also, maybe this needs some automated tests?

Comment 10

[jhp (no longer active)]

The patch includes tests.  They are run when 'make check' is called in the tests directory.  Currently, I think the only tbox that does this for XULRunner, though, is FF-XR tbox in MozillaExperimental.

Comment 11

[jhp (no longer active)]

Attachment: dependency fix

Check in caused a build break on the MozillaExperimental tbox.  The dependencies for MozillaInterfaces.jar aren't set up correctly.  This patch should fix it.  Already checked in to trunk.

Comment 12

[jhp (no longer active)]

Comment on attachment 263398 [details] [diff] [review]
patch

Asking for 1.8.1.4 approval.  This patch is XULRunner only.  One of the files touched is in xpidl, but that bit of code (in xpidl_java) is only exercised when building XULRunner.

This patch is necessary to fix the use of |nsIWebBrowserStream.appendStream()|, an issue that has been reported by several JavaXPCOM users.

Comment 13

[Philippe Ombredanne]

+1 :-P

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 263398 [details] [diff] [review]
patch

This is not something we're going to "slip in" if we need to respin our release candidates, so early next time.

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 263398 [details] [diff] [review]
patch

Who reviewed the xpidl change? I know it's small, but some ownerish person should still put their stamp on it. Looks like dougt, shaver or bsmedberg maybe.

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 263398 [details] [diff] [review]
patch

Would like a review on at least the xpidl changes.

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 263398 [details] [diff] [review]
patch

approved for 1.8.1.5, a=dveditz for release-drivers

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 263398 [details] [diff] [review]
patch

closing tree early, this will have to wait for next time

Comment 19

[Al Billings [:abillings - ex-MoCo]]

Can the original reporter please verify this fix with the 2.0.0.8 RC2 build of Firefox? If it cannot be verified with 2.0.0.8, can QA please be told of a way to verify the fix is present in our code?