nsIBinaryOutputStream readArrayBuffer should return number of bytes read

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: hawkinsw, Assigned: hawkinsw)

Tracking

Trunk
mozilla32
x86_64
Linux
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Assignee

Description

5 years ago
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!
Assignee

Comment 1

5 years ago
Posted 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)
Assignee

Comment 3

5 years ago
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

5 years ago
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)

Updated

5 years ago
Assignee: nobody → hawkinsw
Assignee

Comment 5

5 years ago
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)
Assignee

Comment 6

5 years ago
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
Assignee

Comment 7

5 years ago
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

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

ok. This will need an xpcshell test.
Attachment #8406498 - Flags: feedback?(benjamin) → feedback+
Assignee

Comment 9

5 years ago
Posted patch nsIBinaryInputStream.patch (obsolete) — Splinter Review
Added test case.
Attachment #8406498 - Attachment is obsolete: true
Assignee

Comment 10

5 years ago
Hope that test case meets the expectations!
Assignee

Updated

5 years ago
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+

Updated

5 years ago
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
Assignee

Comment 14

5 years ago
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)
Assignee

Comment 15

5 years ago
(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!
Assignee

Comment 16

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

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

Will
Assignee

Comment 17

5 years ago
And clearing needinfo.
Flags: needinfo?(cbook)
Assignee

Updated

5 years ago
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
Assignee

Comment 23

5 years ago
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)
Assignee

Comment 24

5 years ago
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!
Flags: needinfo?(kwierso)
Assignee

Comment 26

5 years ago
Posted patch nsIBinaryInputStream.patch (obsolete) — Splinter Review
Updated the parameter type.
Attachment #8414700 - Attachment is obsolete: true
Assignee

Comment 27

5 years ago
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.
Assignee

Comment 34

5 years ago
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!
Assignee

Comment 35

5 years ago
Posted patch nsIBinaryInputStream.patch (obsolete) — Splinter Review
With updated UUID.
Attachment #8426768 - Attachment is obsolete: true
Assignee

Comment 36

5 years ago
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)
Posted 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+
Assignee

Comment 39

5 years ago
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!
Assignee

Comment 40

5 years ago
(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.
Assignee

Comment 41

5 years ago
(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)
Assignee

Comment 43

5 years ago
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 ;-)
Assignee

Comment 44

5 years ago
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: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.