Closed Bug 996310 Opened 10 years ago Closed 10 years ago

nsIBinaryOutputStream readArrayBuffer should return number of bytes read

Categories

(Core :: XPCOM, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: hawkinsw, Assigned: hawkinsw)

Details

Attachments

(1 file, 4 obsolete files)

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!
Attached patch nsIBinaryInputStream.patch (obsolete) — Splinter Review
Attachment #8406498 - Flags: review+
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.
Attachment #8406498 - Flags: review+ → feedback?(benjamin)
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 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?
Flags: needinfo?(hawkinsw)
Assignee: nobody → hawkinsw
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
Flags: needinfo?(hawkinsw)
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
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 on attachment 8406498 [details] [diff] [review]
nsIBinaryInputStream.patch

ok. This will need an xpcshell test.
Attachment #8406498 - Flags: feedback?(benjamin) → feedback+
Attached patch nsIBinaryInputStream.patch (obsolete) — Splinter Review
Added test case.
Attachment #8406498 - Attachment is obsolete: true
Hope that test case meets the expectations!
Flags: needinfo?(benjamin)
Comment on attachment 8414700 [details] [diff] [review]
nsIBinaryInputStream.patch

Sorry to have missed this, I've added it to my review queue.
Attachment #8414700 - Flags: review?(benjamin)
Flags: needinfo?(benjamin)
Comment on attachment 8414700 [details] [diff] [review]
nsIBinaryInputStream.patch

Great, thanks for the patch!
Attachment #8414700 - Flags: review?(benjamin) → review+
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: checkin-needed
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
Keywords: checkin-needed
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?
Flags: needinfo?(cbook)
(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!
This Try server is just the coolest thing ever :-)

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

Will
And clearing needinfo.
Flags: needinfo?(cbook)
Flags: needinfo?(cbook)
Will, thanks! setting the checkin-needed flag again so that this can be checkedin
Flags: needinfo?(cbook)
Keywords: checkin-needed
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
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!
Flags: needinfo?(kwierso)
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!
Flags: needinfo?(hawkinsw)
Don't worry, accidents happen!
Attached patch nsIBinaryInputStream.patch (obsolete) — Splinter Review
Updated the parameter type.
Attachment #8414700 - Attachment is obsolete: true
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
Flags: needinfo?(kwierso)
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
Flags: needinfo?(kwierso)
Comment on attachment 8426768 [details] [diff] [review]
nsIBinaryInputStream.patch

Unsure if you need to re-review the change, bsmedberg, but here it is.
Attachment #8426768 - Flags: review?(benjamin)
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 on attachment 8426768 [details] [diff] [review]
nsIBinaryInputStream.patch

r+ in any case, but if the orange re-appears we should clobber before backing out
Attachment #8426768 - Flags: review?(benjamin) → review+
(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 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.
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!
Attached patch nsIBinaryInputStream.patch (obsolete) — Splinter Review
With updated UUID.
Attachment #8426768 - Attachment is obsolete: true
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!
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
Flags: needinfo?(hawkinsw)
Attached patch Rebased patchSplinter Review
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.
Attachment #8427221 - Attachment is obsolete: true
Attachment #8427373 - Flags: review+
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!
(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.
(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.
By default, the "clear needinfo" checkbox should be pre-checked, so you only need to click the "Save changes" button to clear it.
Flags: needinfo?(hawkinsw)
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 ;-)
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.
Flags: needinfo?(kwierso)
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!
Flags: needinfo?(kwierso)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e700f9da7585
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: