850941 - blob size is wrong in open activity after bluetooth image transfer

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[David Flanagan [:djf]]

When I receive an image by bluetooth and open it, the open activity in apps/gallery/js/open.js is called to display it. That code wants to check the size of the file before attempting to open it to prevent OOM errors on very large images.

But the blob.size property is always 18446744073709552000 so all the images appear to be very, very large.

This seems like a gecko or activiies issue, so I probably have the component wrong.

Note that this blocks a bug that is tef+

Comment 1

[David Flanagan [:djf]]

Marking this as blocking bug 848916.

Cc'ing Fabrice and Ben because they always seem to know what to do when blobs go bad.  Also Evelyn and Ian in case this bug is specific to bluetooth.

Comment 2

[David Flanagan [:djf]]

Note that the received blob also has an empty type field. That might or might not be the same bug, however.

Needinfo fabrice, at his request.

Comment 3

[David Flanagan [:djf]]

I've looked at this some more.  I'm using the nightly b2g-18 build from today.

When I do a share activity from the Gallery to the Bluetooth app (both OOP) it works fine, and the receiving app gets a blob with a name, size and type.

But when the system app receives a file by bluetooth, and then uses an open activity to display the file, the same thing fails.  System initiates the activity, passing a file object. Gallery handles the activity, but the blob it receives does not have a name or a type, and its size is always 0x10000000000000180. That's more than 64 bits so something pretty wacky is happening with the size conversion.

The blob itself seems to contain the right data. It just the properties that are broken, and apparently only when transfering from the system app to another app.

I have confirmed when the system app initiates the activity, the file has a valid name, size and type.

Comment 4

[David Flanagan [:djf]]

Given that the blob seems to work as a valid file, I was hoping I could workaround the lack of size with:

blob = new Blob([blob]);

But I get the same size when I do that.

Next I tried new FileReader().readAsArrayBuffer(blob), thinking I could check the size of the array buffer it created. But that just crashed the app.  (OOM maybe since the reported size is so big?)

Comment 5

[David Flanagan [:djf]]

That blob size, 18446744073709552000, is how JavaScript prints Math.pow(2,64). Its the closest JS integer approximation.  It would also be the closest approximation to 2**64 - 1, which would be the value -1 (or any small negative number) in an signed int64.

Comment 6

[[:fabrice] Fabrice Desré]

Talking with David on IRC, the system app gets the file from bluetooth, saves it to device storage, and then shares this file-backed blob. The size is correct in the system app, but gets corrupted afterwards.

Since David has a workaround for now, we are not blocked but more investigation is needed.

Comment 7

[Andrea Marchesini [:baku]]

(In reply to David Flanagan [:djf] from comment #5)
> That blob size, 18446744073709552000, is how JavaScript prints
> Math.pow(2,64). Its the closest JS integer approximation.  It would also be
> the closest approximation to 2**64 - 1, which would be the value -1 (or any
> small negative number) in an signed int64.

When a nsIDOMFile is created, the size is set to UINT64_MAX, so that IsSizeUnknown() returns true. Then, just the first time GetSize() is called, if (isSizeUnknown()), the real size is read from the file.

I think the reason why this bug exists, is because somehow the serialization of blobs process-to-process is broken, or it doesn't share size and type.

This happens only -i'm not 100% sure about this yet- with MozActivity.

/me still debugging it.

Comment 8

[Andrea Marchesini [:baku]]

Attachment: patch

The activity starts from the system app that runs in the main process.
With this patch the blob is created fully (size, date and content type) as it is in the DeviceStorageRequestParent.cpp

Comment 9

[Andrea Marchesini [:baku]]

Attachment: patch

This patch fixes the cursor() and the get() requests.

Comment 10

[Doug Turner (:dougt)]

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

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

shouldn't we do this work in the blob?  lets get bent's review.

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +712,5 @@
> +
> +  nsAutoCString mimeType;
> +  nsCOMPtr<nsIMIMEService> mimeService =
> +    do_GetService(NS_MIMESERVICE_CONTRACTID);
> +  if (mimeService) {

return early if !mimeService

@@ +720,5 @@
> +      return rv;
> +    }
> +  }
> +
> +  mMimeType = NS_ConvertUTF8toUTF16(mimeType);

probably best to set this to something unknown if this function fails.

Comment 11

[Dylan Oliver [:doliver]]

Nominating for 1.1 -- this blocks a leo+ QE1 bug.

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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

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

Looks good to me!

Comment 14

[David Scravaglieri [:scravag]]

It blocks the bug 850542. So blocking

Comment 15

[Eric Chou [:ericchou] [:echou]]

Hi baku,

It seems that the patch has been ready for a few days. Shall we?

Comment 16

[Andrea Marchesini [:baku]]

(In reply to Eric Chou [:ericchou] [:echou] from comment #15)
> Hi baku,
> 
> It seems that the patch has been ready for a few days. Shall we?

It's almost green on try. There is still an orange related to certificates. I hope to see/make it green for today. Then, yes. We can land it.

Comment 17

[Eric Chou [:ericchou] [:echou]]

(In reply to Andrea Marchesini (:baku) from comment #16)
> (In reply to Eric Chou [:ericchou] [:echou] from comment #15)
> > Hi baku,
> > 
> > It seems that the patch has been ready for a few days. Shall we?
> 
> It's almost green on try. There is still an orange related to certificates.
> I hope to see/make it green for today. Then, yes. We can land it.

Thank you, baku.

Comment 18

[Andrea Marchesini [:baku]]

Attachment: m-c patch

Comment 19

[Andrea Marchesini [:baku]]

Attachment: b2g18 patch

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/27c73f18702b

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/27c73f18702b

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/6c712344b003