Last Comment Bug 761159 - FileHandle: Better handling of the end of file state
: FileHandle: Better handling of the end of file state
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Jan Varga [:janv]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-04 08:07 PDT by Jan Varga [:janv]
Modified: 2012-08-07 02:24 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
unaffected
verified
fixed


Attachments
patch (8.10 KB, patch)
2012-06-04 08:19 PDT, Jan Varga [:janv]
no flags Details | Diff | Review
patch (9.30 KB, patch)
2012-06-04 12:40 PDT, Jan Varga [:janv]
jonas: review+
Details | Diff | Review
updated patch (12.98 KB, patch)
2012-06-16 11:03 PDT, Jan Varga [:janv]
jonas: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Review
cast location to uint64_t* (731 bytes, patch)
2012-06-17 05:02 PDT, Landry Breuil (:gaston)
no flags Details | Diff | Review
declare location as uint64_t (722 bytes, patch)
2012-06-17 06:42 PDT, Landry Breuil (:gaston)
jvarga: review+
Details | Diff | Review

Description Jan Varga [:janv] 2012-06-04 08:07:36 PDT

    
Comment 1 Jan Varga [:janv] 2012-06-04 08:19:20 PDT
Created attachment 629786 [details] [diff] [review]
patch

- readAsArrayBuffer(), readAsText() and truncate() throws when the location is in the end of file state (LL_MAXUINT / Infinity)
- write() behaves as append() when in the end of file state
- it's possible to set the state by calling |.location = Infinity|

I'll write a test, but I need feedback on this approach first
Comment 2 Jonas Sicking (:sicking) PTO Until July 5th 2012-06-04 11:44:08 PDT
I'd rather that .write throws when in the end-of-file state. Seems strange that someone would call .append and then .write. That way we are also free to make write-after-append work however we want later if we decide to. I.e. we could change it so that .append doesn't affect .location at all if we decide that's better behavior.

I'd rather make |.location = null| be what puts us in the end-of-file state. That way x.location = x.location is always a no-op.
Comment 3 Jonas Sicking (:sicking) PTO Until July 5th 2012-06-04 11:47:59 PDT
Oh, and we should make .location return null in the end-of-file state. That way we can define it as |attribute long long? location| in the idl once we use WebIDL.
Comment 4 Jan Varga [:janv] 2012-06-04 12:40:40 PDT
Created attachment 629886 [details] [diff] [review]
patch
Comment 5 Jonas Sicking (:sicking) PTO Until July 5th 2012-06-04 12:51:59 PDT
Comment on attachment 629886 [details] [diff] [review]
patch

Review of attachment 629886 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/file/LockedFile.cpp
@@ +516,5 @@
> +  }
> +  else {
> +    if (mLocation <= JSVAL_INT_MAX) {
> +      *aLocation = INT_TO_JSVAL(mLocation);
> +    }

I don't think you need this special-case. Just use JS_NewNumberValue all the time.

@@ +730,5 @@
>    nsresult rv = helper->Enqueue();
>    NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_FILEHANDLE_UNKNOWN_ERR);
>  
>    if (aOptionalArgCount) {
> +    mLocation = aSize;

Wait, this needs to be |mLocation = location|, right? Otherwise mLocation will be set to 0 if the argument isn't specified. Sorry, I missed this in the previous patch that already landed.
Comment 6 Jan Varga [:janv] 2012-06-16 11:03:21 PDT
Created attachment 633831 [details] [diff] [review]
updated patch
Comment 7 Jonas Sicking (:sicking) PTO Until July 5th 2012-06-16 16:39:37 PDT
Comment on attachment 633831 [details] [diff] [review]
updated patch

Review of attachment 633831 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

::: dom/file/LockedFile.cpp
@@ +533,5 @@
> +    return NS_OK;
> +  }
> +
> +  PRUint64 location;
> +  if (xpc::ValueToUint64(aCx, aLocation, &location)) {

Nit: I would have reversed the check here and put the error handling in the branch. As to keep the common code flow to be the one that reaches the end of the function. Up to you.

@@ +708,5 @@
> +  PRUint64 location;
> +  if (aOptionalArgCount) {
> +    // Just in case someone calls us from C++
> +    if (aSize == LL_MAXUINT) {
> +      return NS_ERROR_TYPE_ERR;

I'd prefer to just MOZ_ASSERT that aSize != LL_MAXUINT. That's a better way to keep people from doing that.
Comment 8 Jan Varga [:janv] 2012-06-17 00:40:49 PDT
Both comments addressed.

http://hg.mozilla.org/mozilla-central/rev/0ee6b0ed0446
Comment 9 Landry Breuil (:gaston) 2012-06-17 03:28:46 PDT
This breaks my OpenBSD builds..

dom/file/LockedFile.cpp:537: error: invalid conversion from 'PRUint64*' to 'uint64_t*'

xpc::ValueToUint64 expects a uint64_t* as last arg and gets a PRUint64*. Trying some casting vodoo..
Comment 10 Jonas Sicking (:sicking) PTO Until July 5th 2012-06-17 03:53:38 PDT
Comment on attachment 633831 [details] [diff] [review]
updated patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
Bug 726593

User impact if declined: 
Would result in that web developers would have a somewhat inconsistent behavior for the new FileHandle API. Ideally developers would be able to stop worrying about this after 6 weeks when FF16 is released, but we're still not great at getting users to the latest version.

I.e. we'd like to fix known issues in the first release of this API.

Testing completed (on m-c, etc.): 
Patch includes thorough automated tests.

Risk to taking this patch (and alternatives if risky):
Very low risk. Only affects an API which was added in Firefox 15, so no existing websites or internal code uses it.

Absolute worst case this API doesn't work in Firefox 15, but even that is very unlikely since the patch only affects edge cases and is will testsed.

String or UUID changes made by this patch: 
Changes the UUID of an interface introduced in Firefox 15.
No string changes.
Comment 11 Landry Breuil (:gaston) 2012-06-17 05:02:02 PDT
Created attachment 633888 [details] [diff] [review]
cast location to uint64_t*

Attached patch fixes the build for me, dunno if that's the best fix.
Comment 12 Jonas Sicking (:sicking) PTO Until July 5th 2012-06-17 05:05:03 PDT
Comment on attachment 633888 [details] [diff] [review]
cast location to uint64_t*

I think it'd be better to change 'location' to be of type uint64_t. And, only if needed, add a cast when assigning into mLocation.
Comment 13 Jan Varga [:janv] 2012-06-17 05:46:15 PDT
yeah, could you try something like:

uint64_t location;
if (!xpc::ValueToUint64(aCx, aLocation, &location)) {
  return NS_ERROR_TYPE_ERR;
}

mLocation = location; // eventually change to: mLocation = static_cast<PRUint64>(location);
Comment 14 Landry Breuil (:gaston) 2012-06-17 06:42:43 PDT
Created attachment 633895 [details] [diff] [review]
declare location as uint64_t

That also works, and it even seems the cast when assigning to mLocation isnt needed.
Comment 16 Ed Morley [:emorley] 2012-06-18 07:28:43 PDT
https://hg.mozilla.org/mozilla-central/rev/449cfd3737cd
Comment 17 Alex Keybl [:akeybl] 2012-06-19 20:14:31 PDT
Comment on attachment 633831 [details] [diff] [review]
updated patch

[Triage Comment]
Risk and UUID changes are limited to the new API. Approving for Aurora 15.
Comment 18 Landry Breuil (:gaston) 2012-06-22 08:15:21 PDT
To whoever commits that to aurora (i can, if needed...) dont forget to also take https://hg.mozilla.org/mozilla-central/rev/449cfd3737cd :)

Note You need to log in before you can comment on or make changes to this bug.