996310 - nsIBinaryOutputStream readArrayBuffer should return number of bytes read

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Will Hawkins]

readArrayBuffer in nsIBinaryInputStream throws NS_ERROR_FAILURE when the number of bytes requested cannot be read from the underlying input stream. This is a problem when the underlying stream is network-based and it is not uncommon for read() to return fewer than the number of bytes requested. 

This patch adds a return value to the function to indicate the number of bytes read from the buffer. The patch changes the IDL appropriately but it does not address the necessary changes for unit tests. I'm still new to the Mozilla developer community, but I know those are required :-) 

Please let me know what I can improve about this patch. I'm excited to have the chance to contribute!

Comment 1

[Will Hawkins]

Attachment: nsIBinaryInputStream.patch

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8406498 [details] [diff] [review]
nsIBinaryInputStream.patch

Thanks for the patch, Will!

We typically use the review+ flag to indicate that a patch has been reviewed by somebody familiar with the code, not as a request for a patch to be reviewed.  If you'd like somebody to look at a patch, you can set the review? flag and type in a person's name or email address; Bugzilla will also provide a "suggested reviewers" link for who would be appropriate to flag.  I'll clear the review+ flag for you.

Since it sounds like the patch might not be ready for review, I've asked Benjamin Smedberg to provide feedback on your patch.

Comment 3

[Will Hawkins]

Hey Nathan!

Thanks for helping me understand that flag. I "checked that box" intentionally, but by accident. I thought I was setting the "ready for review"/"request for review". I won't make the same mistake again. 

Thanks again for helping me with the process!

Will

Comment 4

[Benjamin Smedberg]

Comment on attachment 8406498 [details] [diff] [review]
nsIBinaryInputStream.patch

My big question about this is how this will affect existing clients.

http://dxr.mozilla.org/mozilla-central/search?q=readarraybuffer&redirect=true is a list of possible existing clients, and the fact that existing code might *expect* an exception worries me a bit, especially because that's part of `fs` and `stream` in the addon SDK which means that the extension ecosystem might be affected.

Do you have an example of where in the tree this would help you, so that I can understand the reward part of the reward/risk ratio better?

Comment 5

[Will Hawkins]

Hello Benjamin!

I'm definitely willing to go through the code to find the possible users of this. 

I found this caveat in the process of plugin development so I'm glad that you take into account those users too :-) The upside is that this particular version is not even in the public documentation yet. So, it's possible that the number of existing add-on users is limited.
 
It would be okay to simply throw an exception except (pun intended) for the fact that the partially read data is copied from the underlying stream to the arraybuffer. I looked for a way to manipulate the arraybuffer object by setting it's length property (or somesuch) as a way to give alternate feedback to the caller when the exception is thrown. The length properties for an arraybuffer are read-only and I couldn't find any other acceptable alternative. Is there something that I missed, perhaps? I'm very new to the codebase so please feel free to tell me that I've missed something obvious!

Thank you so much for engaging with me on this. Having a way to use this function in the presence of partial reads is very important for our plugin.

Will

Comment 6

[Will Hawkins]

It seems that of the examples found in the mxr search, the only one that checks for an exception from readArrayBuffer is in toolkit/modules/ZipUtils.jsm. In that file, the caller of readArrayBuffer checks for an exception generated when the underlying stream is closed and then generically handles other exceptions. 

Obviously this does not mean that functions calling the ones referenced in these search results do not check for exceptions that bubble up from within the called functions. 

Will

Comment 7

[Will Hawkins]

Hello!

I was just wondering if there was anything I could do to help this through. Just let me know! 

And, thanks for your patience with a new contributor.

Will

Comment 8

[Benjamin Smedberg]

Comment on attachment 8406498 [details] [diff] [review]
nsIBinaryInputStream.patch

ok. This will need an xpcshell test.

Comment 9

[Will Hawkins]

Attachment: nsIBinaryInputStream.patch

Added test case.

Comment 10

[Will Hawkins]

Hope that test case meets the expectations!

Comment 11

[Benjamin Smedberg]

Comment on attachment 8414700 [details] [diff] [review]
nsIBinaryInputStream.patch

Sorry to have missed this, I've added it to my review queue.

Comment 12

[Benjamin Smedberg]

Comment on attachment 8414700 [details] [diff] [review]
nsIBinaryInputStream.patch

Great, thanks for the patch!

Comment 13

[Carsten Book [:Tomcat]]

Hi,
could you provide a Try link. Suggestions for what to run if you haven't
yet can be found here:
https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices

Comment 14

[Will Hawkins]

I would love to do this! However, I do not yet have committer access to get on the Try server. I am going through the process for that account now. Is there something I can do in the meantime?

Comment 15

[Will Hawkins]

(In reply to Will Hawkins from comment #14)
> I would love to do this! However, I do not yet have committer access to get
> on the Try server. I am going through the process for that account now. Is
> there something I can do in the meantime?

If you could vouch, the bug number is 1011634. 

Thanks!

Comment 16

[Will Hawkins]

This Try server is just the coolest thing ever :-)

My submission is at https://tbpl.mozilla.org/?tree=Try&rev=6357862ee79d

Will

Comment 17

[Will Hawkins]

And clearing needinfo.

Comment 18

[Carsten Book [:Tomcat]]

Will, thanks! setting the checkin-needed flag again so that this can be checkedin

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/16418be9d4dd

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Argh, I had to rebase this and let a stray + get in there. Pushed a bustage follow-up.
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c23781bad3c

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed both out in https://hg.mozilla.org/integration/mozilla-inbound/rev/94b9062d4629for mochitest-dt1 bustage: https://tbpl.mozilla.org/php/getParsedLog.php?id=40135409&tree=Mozilla-Inbound

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I also want to blame this patch on the Jetpack test failures: https://tbpl.mozilla.org/php/getParsedLog.php?id=40139109&tree=Mozilla-Inbound

And the XPCShell test failures: https://tbpl.mozilla.org/php/getParsedLog.php?id=40139124&tree=Mozilla-Inbound

Comment 23

[Will Hawkins]

I am completely embarassed. I will look through those failures and see if I can get to the bottom of this. At first glance I don't see how my change could have caused such a problem, but it obviously did. I will look into it. I'm very sorry!

Comment 24

[Will Hawkins]

I think that I found the problem. I accidentally changed the parameter type of the array buffer itself in the process of making this patch. I rolled another patch and just put it on the try server. I will send a result when it's ready. Again, I'm really sorry!

Comment 25

[Andrew McCreight [:mccr8]]

Don't worry, accidents happen!

Comment 26

[Will Hawkins]

Attachment: nsIBinaryInputStream.patch

Updated the parameter type.

Comment 27

[Will Hawkins]

Plus try server output for the tests that previously failed:

https://tbpl.mozilla.org/?tree=Try&rev=86e4b17ab030

Jetpack and xpcshell on win32.

What concerns me is that JS::HandleValue should be a typedef for JS::Handle<JS::Value> so changing it should not make a difference. The only other possibility is that the return parameter is not being passed correctly to the function. But, the invoking code should setup those return values for me, right? Based on other xpcom code, that seems to be the case. 

Thanks for bearing with me!

Will

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I would actually be a bit more aggressive with your try run, just to make sure you've caught everything. This should be sufficient for your try syntax:
try: -b do -p macosx64,win32 -u xpcshell,jetpack,mochitest-dt -t none

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Comment on attachment 8426768 [details] [diff] [review]
nsIBinaryInputStream.patch

Unsure if you need to re-review the change, bsmedberg, but here it is.

Comment 30

[Benjamin Smedberg]

The only difference between this patch and the last is that you went "back" to JS::Handle<JS::Value> instead of using JS::HandleValue?

Those are typedefs of the same thing and should be strictly equivalent:

<bsmedberg> Are JS::Handle<JS::Value> and JS::HandleValue equivalent?
<terrence-afkish> bsmedberg: yes

The crash at https://tbpl.mozilla.org/php/getParsedLog.php?id=40135409&tree=Mozilla-Inbound#error1 is at this stack:

https://hg.mozilla.org/integration/mozilla-inbound/file/8c23781bad3c/xpcom/io/nsBinaryStream.cpp#l409
https://hg.mozilla.org/integration/mozilla-inbound/file/8c23781bad3c/xpcom/io/nsBinaryStream.cpp#l835

So I don't think it has anything to do with the HandleValue, and instead somehow xpconnect is passing a null rLength.

I'm willing to blame dep-build IDL/XPT bugs for this and think we should try landing the original patch (rebased correctly).

Comment 31

[Benjamin Smedberg]

Comment on attachment 8426768 [details] [diff] [review]
nsIBinaryInputStream.patch

r+ in any case, but if the orange re-appears we should clobber before backing out

Comment 32

[Nathan Froyd [:froydnj]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #30)
> I'm willing to blame dep-build IDL/XPT bugs for this and think we should try
> landing the original patch (rebased correctly).

Probably need a UUID update on nsIBinaryInputStream.idl.

Comment 33

[Benjamin Smedberg]

Comment on attachment 8426768 [details] [diff] [review]
nsIBinaryInputStream.patch

Oh, yes! That is necessary. Will you'll need to generate a new random UUID for this IDL.

Comment 34

[Will Hawkins]

YES! Thank you so much Nathan and Benjamin! This makes perfect sense. As you said, the change I made should have had 0 effect (since they're typedefs). 

So, just to try to make sense of this: when I generate a new UUID for this IDL it will regenerate the wrapper/invocation code to make sure that it gives me a valid "return value" as the final parameter? 

I will update the UUID and then respin a patch!

Comment 35

[Will Hawkins]

Attachment: nsIBinaryInputStream.patch

With updated UUID.

Comment 36

[Will Hawkins]

Thanks to Benjamin and Nathan, I regenerated a UUID for this IDL (I followed the docs and used the online PERL script to do so). 

I also followed Wes' advice and pushed to the TryServer with a more aggressive syntax. Here is a link to the results of those tests:

https://tbpl.mozilla.org/?tree=Try&rev=a21ac32f21b9

As I said previously, I really appreciate everyone's help with this patch process! I'm sorry that it has been such a problem!

Comment 37

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

It looks like your repository's code that your patch is based on is a pretty old checkout from sometime in April, so it no longer agrees with the way the build system is set up, causing those tests to fail. 

I rebased your patch (there were some minor formatting conflicts in xpcom/io/nsBinaryStream.cpp because that file's been updated since your patch was written) on top of the current mozilla-central revision and pushed it to try as https://tbpl.mozilla.org/?tree=Try&rev=581f27debcb8

The rebased patch is at https://hg.mozilla.org/try/rev/5954158b3834

Comment 38

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: Rebased patch

Try run's looking decent so far.

Here's the rebased patch for whomever handles the checkin-needed later. Carrying forward bsmedberg's r+, assuming nothing needed to be re-reviewed in this.

Comment 39

[Will Hawkins]

Thanks for rebasing my patch, Wes. I will definitely stay more up-to-date with the codebase so that future patches are doing against recent versions of the code. I will continue to keep an eye on the Try to make sure that it finishes successfully!

Comment 40

[Will Hawkins]

(In reply to Will Hawkins from comment #39)
> Thanks for rebasing my patch, Wes. I will definitely stay more up-to-date
> with the codebase so that future patches are doing against recent versions
> of the code. I will continue to keep an eye on the Try to make sure that it
> finishes successfully!

Unsetting needinfo.

Comment 41

[Will Hawkins]

(In reply to Will Hawkins from comment #40)
> (In reply to Will Hawkins from comment #39)
> > Thanks for rebasing my patch, Wes. I will definitely stay more up-to-date
> > with the codebase so that future patches are doing against recent versions
> > of the code. I will continue to keep an eye on the Try to make sure that it
> > finishes successfully!
> 
> Unsetting needinfo.

For real.

Comment 42

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

By default, the "clear needinfo" checkbox should be pre-checked, so you only need to click the "Save changes" button to clear it.

Comment 43

[Will Hawkins]

Thanks! It's really just not my night. Or week. I wouldn't blame you if you all decided never to accept another patch from me ;-)

Comment 44

[Will Hawkins]

Try results are in and look decent. The only failure is 

20:26:45  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/framework/test/browser_toolbox_options_disable_js.js | Output is "JavaScript Enabled" in iframe - Got No output, expected JavaScript Enabled

Which does not appear related to this patch.

Comment 45

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Yep, I think this is good to go back into the checkin-needed list. I'll try to get another round of checkin-needed patches in before I head out tonight, but no guarantees.

Thanks for following through with this patch, Will!

Comment 46

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e700f9da7585

Comment 47

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e700f9da7585