Closed Bug 812412 Opened 11 years ago Closed 11 years ago

[b2g-bluetooth] the header "Name" should only contain file name, not file path

Categories

(Core :: DOM: Device Interfaces, defect)

Product:
Core
Component:
DOM: Device Interfaces
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla20
Project Flags:
blocking-basecamp +
Tracking Flags:
Tracking Status
firefox18 --- fixed
firefox19 --- fixed
firefox20 --- fixed

People

(Reporter: echou, Assigned: echou)

References

Details

(Whiteboard: [LOE:S])

Attachments

(1 file, 2 obsolete files)

patch 1: final: replace full file path with leaf name, r=dougt
1.43 KB, patch
Details | Diff | Splinter Review 
I tested file-sharing function with my HTC Sensation XL and it worked well. However, I failed to send a file to my laptop (Ubuntu 12.04).

The root cause is the incorrect Name header. According to section 9.2.2 [Commonly Used Headers] of IrOBEX v1.2 spec, "the Name header should not contain any path information", my laptop would refuse our file sending request with error code "Unauthorized".
Hi Doug, could you please help me on this review? I can confirm that the logic of this patch is correct, but I don't know if there is a better way to get leaf name of a nsIDOMFile object. 

Thanks in advance.
Assignee: nobody → echou
Attachment #682340 - Flags: review?(doug.turner)
Blocks: 792683
blocking-basecamp: --- → ?
Whiteboard: [LOE:S]
a better approach would be to add a new method on nsIDOMFile to expose the leafname (which is different than the |name| you're using.

You could add something like this:

[noscript] readonly attribute uint64_t mozLeafName;


bent or sicking should let you know if they want to add this attribute to the interface.


Alternatively, you *could* use mozFullPathInternal and then do the path parsing yourself, but that logic really belongs in nsLocalFile.
Flags: needinfo?(jonas)
a better approach would be to add a new method on nsIDOMFile to expose the leafname (which is different than the |name| you're using.

You could add something like this:

[noscript] readonly attribute uint64_t mozLeafName;


bent or sicking should let you know if they want to add this attribute to the interface.


Alternatively, you *could* use mozFullPathInternal and then do the path parsing yourself, but that logic really belongs in nsLocalFile.
Adding a mozLeafName would be ok with me. As would sticking with the current patch.

I do think that there is general agreement that .name can represent a name within a sub-tree, though the unclarity around filesystems right now doesn't really make it very certain where we're going to end up with regards to that.
Flags: needinfo?(jonas)
Eric, you want to add the new attribute?
Unclear exactly what the consequences of not fixing this is, so just making this a soft-blocker.
blocking-basecamp: ? → +
Whiteboard: [LOE:S] → [LOE:S][soft-blocker]
@Jonas
Sorry for unclear description. We can't send file from B2G phone to many devices if this is not fixed. So this should be a blocker.

@Doug
I prefer not to add a new attribute since I think there are other ways to achieve the goal. So I'm going to use your mozFullPathInternal/nsLocalFile approach to solve this problem. That should work in different platform.

Thank you.
Used the file path returned by calling GetMozFullPathInternal() to create a nsLocalFile object, then called GetLeafName() to get the file name.
Attachment #682340 - Attachment is obsolete: true
Attachment #682340 - Flags: review?(doug.turner)
Attachment #683471 - Flags: review?(doug.turner)
(In reply to Jonas Sicking (:sicking) from comment #6)
> Unclear exactly what the consequences of not fixing this is, so just making
> this a soft-blocker.

If this bug really blocks bug 812715, we need to make this a P1-major blocker, not just a [softblocker].   bug 812715 is preventing any device to bluetooth transfer files with each other.
(In reply to Tony Chung [:tchung] from comment #9)
> (In reply to Jonas Sicking (:sicking) from comment #6)
> > Unclear exactly what the consequences of not fixing this is, so just making
> > this a soft-blocker.
> 
> If this bug really blocks bug 812715, we need to make this a P1-major
> blocker, not just a [softblocker].   bug 812715 is preventing any device to
> bluetooth transfer files with each other.

I know many devices can't receive files sent from us because of this issue, so it should be a blocker, but I'm not sure if this is the root cause of bug 812715. It would be nice to test again using the same remote device and with this patch applied.
Whiteboard: [LOE:S][soft-blocker] → [LOE:S]
Comment on attachment 683471 [details] [diff] [review]
patch 1: v2: replace full file path with leaf name

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

r+ with my changes.

if mBlob is always a nsIDOMFile, i'd rather you assert instead of just checking for null.

::: dom/bluetooth/BluetoothOppManager.cpp
@@ +282,5 @@
>     *  - After receiving the response, start to read file and send.
>     */
>    mBlob = aActor->GetBlob();
>  
> +  nsCOMPtr<nsIDOMFile> domFile = do_QueryInterface(mBlob);

is mBlob always a nsIDOMFile?

@@ +297,1 @@
>    }

Just do:

nsCOMPtr<nsIDOMFile> domFile = do_QueryInterface(mBlob);

if (domFile && NS_SUCCEEDED(domFile->GetMozFullPathInternal(fullPath))) {
  nsCOMPtr<nsIFile> localFile;
  NS_NewLocalFile(fullPath, false, getter_AddRefs(localFile));
  if (localFile) {
    localFile->GetLeafName(sFileName);
  }
}
Attachment #683471 - Flags: review?(doug.turner) → review+
(In reply to Doug Turner (:dougt) from comment #11)
> Comment on attachment 683471 [details] [diff] [review]
> patch 1: v2: replace full file path with leaf name
> 
> Review of attachment 683471 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with my changes.
> 
> if mBlob is always a nsIDOMFile, i'd rather you assert instead of just
> checking for null.
> 
> ::: dom/bluetooth/BluetoothOppManager.cpp
> @@ +282,5 @@
> >     *  - After receiving the response, start to read file and send.
> >     */
> >    mBlob = aActor->GetBlob();
> >  
> > +  nsCOMPtr<nsIDOMFile> domFile = do_QueryInterface(mBlob);
> 
> is mBlob always a nsIDOMFile?
> 

No, it's not. I can handle nsIDOMBlob object as well, so the check is necessary.
* Used NS_NewLocalFile() to create an nsLocalFile object with nsString file path
Attachment #683471 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/2a35b6fb48d7
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
This will produce files with "unknown" name more often than the v1 patch. There are ways of getting a File object with a .name like "foo/bar.txt" but where GetMozFullPathInternal will return the empty string.

In particular, if you store a File with .name="foo/bar.txt" in IndexedDB and then retrieve it, you'll get a File object with .name="foo/bar.txt", but where GetMozFullPathInternal will return empty string I think. Or possibly will return the name of the internal file as stored in the indexedDB implementation, which will be completely unrelated to the value of .name.

That said, it doesn't seem like a blocker if we return "unknown" or a random value, as long as the transfer succeeds, which it now should.
(In reply to Jonas Sicking (:sicking) from comment #17)
> This will produce files with "unknown" name more often than the v1 patch.
> There are ways of getting a File object with a .name like "foo/bar.txt" but
> where GetMozFullPathInternal will return the empty string.
> 
> In particular, if you store a File with .name="foo/bar.txt" in IndexedDB and
> then retrieve it, you'll get a File object with .name="foo/bar.txt", but
> where GetMozFullPathInternal will return empty string I think. Or possibly
> will return the name of the internal file as stored in the indexedDB
> implementation, which will be completely unrelated to the value of .name.
>

The problem of v1 patch is that it can't handle different representation of 
file path for different platforms. Only if file path is separated by '/'
will be handled correctly.

> That said, it doesn't seem like a blocker if we return "unknown" or a random
> value, as long as the transfer succeeds, which it now should.

Sending file with name "unknown" is more likely to be rejected by remote devices.
It depends on the implementation of remote devices. It works if my laptop is the
target device, but it failed on my HTC Sensation XL. That's why we try so hard to
get file name.

Currently I can get entire path in sendFile() by GetMozFullPathInternal() since 
there are only Gallery and Music supporting Bluetooth file-sharing. It may be
worth creating a followup bug for this.
(In reply to Eric Chou [:ericchou] [:echou] from comment #18)
> The problem of v1 patch is that it can't handle different representation of 
> file path for different platforms. Only if file path is separated by '/'
> will be handled correctly.

The .name property is normalized to always use "/" as separator on all platforms. So that shouldn't be a problem.

> > That said, it doesn't seem like a blocker if we return "unknown" or a random
> > value, as long as the transfer succeeds, which it now should.
> 
> Sending file with name "unknown" is more likely to be rejected by remote
> devices.
> It depends on the implementation of remote devices. It works if my laptop is
> the
> target device, but it failed on my HTC Sensation XL. That's why we try so
> hard to
> get file name.

Do these literally look for the string "unknown" and reject it? Or are they requiring a filename like "foo.bar"?

How do these devices know that the user isn't trying to transfer a file which actually happens to be called "unknown"?

Can we use a different fallback name that is less likely to be rejected?
(In reply to Jonas Sicking (:sicking) from comment #20)
> (In reply to Eric Chou [:ericchou] [:echou] from comment #18)
> > The problem of v1 patch is that it can't handle different representation of 
> > file path for different platforms. Only if file path is separated by '/'
> > will be handled correctly.
> 
> The .name property is normalized to always use "/" as separator on all
> platforms. So that shouldn't be a problem.
> 

Oh, I didn't notice that. Thanks for reminding. :D

In this case, I think the v1 patch would be better since it's more straightforward. I'll file a followup bug for this.

> > > That said, it doesn't seem like a blocker if we return "unknown" or a random
> > > value, as long as the transfer succeeds, which it now should.
> > 
> > Sending file with name "unknown" is more likely to be rejected by remote
> > devices.
> > It depends on the implementation of remote devices. It works if my laptop is
> > the
> > target device, but it failed on my HTC Sensation XL. That's why we try so
> > hard to
> > get file name.
> 
> Do these literally look for the string "unknown" and reject it? Or are they
> requiring a filename like "foo.bar"?
> 

Yes, I guess those devices don't like the name without extension.

> How do these devices know that the user isn't trying to transfer a file
> which actually happens to be called "unknown"?
> 

I think this kind of requests would also be rejected. :|

> Can we use a different fallback name that is less likely to be rejected?

I've tried "noname" and "temp". Both of them did not work. Also, I've tried to name a photo "Unknown.jpg" and successfully sent it. Therefore I guess the main problem is related to the lack of extension, not the naming.
Yeah, sounds like the extension is the problem.

What we *ideally* should do is something like this:

name = ""
if (blob is of type File) {
  name = getLeafName(blob.name)
}
if (name == "") {
  name = localize("unknown")
}
if (!hasExtension(name)) {
  name = name + "." + nsIMimeService.getPrimaryExtension(blob.type, "");
}
follow-up: Bug 814376
You need to log in before you can comment on or make changes to this bug.