1079649 - [MediaDB]ConstraintError is not handled in case of firstscan is true

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[vsireesha246]

Steps to Reproduce:

Preconditions:
1.Flash the Binary
2.BT Pairing should be done

STR:
1.Open video app without any videos
2.Add videos to get started overlay should be shown
3.Press the Home key to make Video app in background
4.Receive any video file from the any other BT paired device
5.If the BT file receiving is in progress(half of the receiving is done),Close the Video app and reopen it and check the video thumbnail size and make the video app in foreground.
6.After the BT receive is complete check the thumbnail video file size.Its not updated to actual size.
7.If we kill the Video app and reopen it,the size is updated properly.

Actual: The Size is not updated properly
Expected:The Size is not updated properly without killing the app.

The same behavior is also present in Gallery app as well.

Comment 1

[vsireesha246]

Attachment: Bug_1079649.patch

Please find the attached work-around patch for this issue.

Comment 2

[vsireesha246]

Hi Vance Chen,

Would you please help me to review this patch.

Thanks..
Sireesha

Comment 3

[vsireesha246]

This issue is present in V2.0 and Master as well.
When ever this issue is reproduced "ConstraintError" due to file already exists in the database.

Comment 4

[vsireesha246]

(In reply to vsireesha246 from comment #0)
> Steps to Reproduce:
> 
> Preconditions:
> 1.Flash the Binary
> 2.BT Pairing should be done
> 
> STR:
> 1.Open video app without any videos
> 2.Add videos to get started overlay should be shown
> 3.Press the Home key to make Video app in background
> 4.Receive any video file from the any other BT paired device
> 5.If the BT file receiving is in progress(half of the receiving is
> done),Close the Video app and reopen it and check the video thumbnail size
> and make the video app in foreground.
> 6.After the BT receive is complete check the thumbnail video file size.Its
> not updated to actual size.
> 7.If we kill the Video app and reopen it,the size is updated properly.
> 
> Actual: The Size is not updated properly
> Expected:The Size should be updated properly without killing the app.
> 
> The same behavior is also present in Gallery app as well.

Comment 5

[vsireesha246]

Hi Eric and Dave,

Would you please help me to find the issue is in Gecko or Gaia side.
The attached patch will solve the issue and it may cause performance issues.
So its better to handle in Gecko side?

Thanks..
Sireesha

Comment 6

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

This is more likely to be related to the thumbnail generation mechanism but not Bluetooth. Clear my ni and feel free to ni me again if you have any bt questions.

Comment 7

[vsireesha246]

Attachment: Bug_1079649.patch

Patch modified moved media.details.firstscan = false; to scanend().

With this patch this issue got resolved in Gallery,Music and Video app.
But i found below issues.

1.In Gallery app->if the uncompleted BT transfer file is opened,once the transfer completes,its coming from full screen view to thumbnail view.

2.In video app once the BT transfer completes "Empty Vidoes" overlay shows for a moment and then thumbnail size will be updated.

Comment 8

[David Flanagan [:djf]]

It sounds like the bluetooth download code is making partially downloaded files visible to the media apps. This is a very dangerous thing since it means we can scan partially complete files. This could easily lead to metadata problems.  If the file is an old-style MP3 with the metadata at the end, for example, we would fail to find it.

Dave: are you familiar with the bluetooth download code? Can we make it not advertise the new files on device storage until they are complete?  Actually, this is probably not just a matter of not sending the "modified" event. The files should be downloaded with a temporary name and not renamed until the download is complete. If the temporary name does not have a file extension the files will not be recognized by any of the media device storages.

Eric: is my analysis of the bluetooth download mechanism correct here? Can it be modified to rename files once the download is complete?

Comment 9

[Dave Hylands [:dhylands]]

Doing a fullscan is a definite performance hit.

The real solution is to find the source of the ConstraintError. Doing a fullscan just sidesteps the issue.

Putting ni? on djf since he may know what's causing the ConstraintError (sounds like IndexedDB?)

Comment 10

[David Flanagan [:djf]]

Dave: see comment 8 for my thoughts on this.  I was hoping you'd see that as part of your needinfo response.

I don't know where the ConstraintError is coming from, but I'm completely freaked out by the fact that these partially downloaded files are being exposed to the media apps. That's a guarantee of problems down the road. To fix this, I think we need to change the bluetooth code so that device storage never knows about the files until they are complete. The reporter contacted me by IRC and agrees that fixing this in gecko would be much better than the proposed gaia patch.

I'm assuming that fixing it in gecko would make the constraint error go away and we can just ignore the title of this bug and the particular symptoms described here.

Comment 11

[vsireesha246]

If we handle this "ConstraintError" in mediadb.js like the attached patch,as mentioned in #comment7 i found these issues.

1.In video app
In mediadb.js file already "ConstraintError" error is handled..and once it occurs we are deleting the file and recreating it.
This causes in this case "Empty video overlay" shows a movement and then the actual file is will be updated in thunbnaillist and its looks annoying to user.
and also if user is playing incomplete video file and the BT receive completes,then we are deleting the file causing "Empty video overlay" always shown in video app.

2.In case of Gallery app (Image file)
If the user opens the imcomplete image file and it is in fullscreen view,once the receive completes,the fullscreenview is changed to thuumbnailview.

Comment 12

[Dave Hylands [:dhylands]]

Dave - Sorry I just looked at the patch.

Yeah - in the BT code we should change it to do the following:

1 - Download the file to foo.jpg.part
2 - When the download is finished rename foo.jpg.part to foo.jpg
3 - Then send out the "modified" event.

I'm going to file a similar bug against MTP.

Comment 13

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

(In reply to Dave Hylands [:dhylands] from comment #12)
> Dave - Sorry I just looked at the patch.
> 
> Yeah - in the BT code we should change it to do the following:
> 
> 1 - Download the file to foo.jpg.part
> 2 - When the download is finished rename foo.jpg.part to foo.jpg
> 3 - Then send out the "modified" event.
> 
> I'm going to file a similar bug against MTP.

Fair enough. I'll take this bug.

Comment 14

[Rachelle Yang [:ryang][[e-mail]]]

Hi Eric, Could you please kindly share the status of the bug ?
Any comments on the patch in comment7 ?
Thank you very much !!

Comment 15

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

Attachment: patch 1: v1: Use a temporary (.part) file to store incoming file

Hi Dave,

Would you mind taking a look of my patch? The main ideas of my patch are:

(1) Create a temp file object(mTempDsFile) to occupy the unique file name
(2) Create another file object(mDsFile) to receive the file content.
(3) After the file has been received completely, delete mTempDsFile and rename the file name of mDsFile to the one mTempDsFile original has.

I'll ask for another review from Bluetooth peer, so please ignore Bluetooth related logic. Thanks!

Comment 16

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

(In reply to Rachelle Yang [:ryang][[e-mail]] from comment #14)
> Hi Eric, Could you please kindly share the status of the bug ?
> Any comments on the patch in comment7 ?
> Thank you very much !!

Rachelle,

The patch I provided could be directly applied to 2.0 codebase. Please feel free to ask partners to take it and give it a try if they want to.

Thanks.

Comment 17

[Dave Hylands [:dhylands]]

Comment on attachment 8506078 [details] [diff] [review]
patch 1: v1: Use a temporary (.part) file to store incoming file

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

::: dom/bluetooth/bluedroid/BluetoothOppManager.cpp
@@ +603,5 @@
> +  path.Append(mFileName);
> +  path.AppendLiteral(".part");
> +
> +  mDsFile =
> +    DeviceStorageFile::CreateUnique(path, nsIFile::NORMAL_FILE_TYPE, 0644);

I think that this causes mDsFile to get the name with the .part extension.

Then later in NotifyAboutFileChange, I think that it will still have the .part extension.

I'd also be inclined to move the unique filename creation to the end (so then we just create the .part and don't even bother creating the unique filename until the very end (so we don't have a zero length file either).

So I think I would add .part and call CreateUnique here. Then at the end take the original filename and call CreateUnique again and then do the rename. Then we don't wind up with a zero length file.

Although looking at the download code:
http://dxr.mozilla.org/mozilla-central/source/b2g/components/HelperAppDialog.js#78

it seems to create the empty file as well. So I'm ok with leaving this.

I think you should test with ds-test app (choose AddListener, press DoIt) then tranfer via bluetooth and make sure ds-test reports the correct filename.

Comment 18

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

(In reply to Dave Hylands [:dhylands] from comment #17)
> Comment on attachment 8506078 [details] [diff] [review]
> patch 1: v1: Use a temporary (.part) file to store incoming file
> 
> Review of attachment 8506078 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/bluedroid/BluetoothOppManager.cpp
> @@ +603,5 @@
> > +  path.Append(mFileName);
> > +  path.AppendLiteral(".part");
> > +
> > +  mDsFile =
> > +    DeviceStorageFile::CreateUnique(path, nsIFile::NORMAL_FILE_TYPE, 0644);
> 
> I think that this causes mDsFile to get the name with the .part extension.
> 
> Then later in NotifyAboutFileChange, I think that it will still have the
> .part extension.
> 

The file name of mDsFile would be renamed to mFileName before NotifyAboutFileChange was called, so the file name which was embedded in the notification shouldn't contain the .part extension.

> I'd also be inclined to move the unique filename creation to the end (so
> then we just create the .part and don't even bother creating the unique
> filename until the very end (so we don't have a zero length file either).
> 
> So I think I would add .part and call CreateUnique here. Then at the end
> take the original filename and call CreateUnique again and then do the
> rename. Then we don't wind up with a zero length file.
> 

That should also work.

> Although looking at the download code:
> http://dxr.mozilla.org/mozilla-central/source/b2g/components/HelperAppDialog.
> js#78
> 
> it seems to create the empty file as well. So I'm ok with leaving this.
> 

ok.

> I think you should test with ds-test app (choose AddListener, press DoIt)
> then tranfer via bluetooth and make sure ds-test reports the correct
> filename.

Oh, good to know there is a test tool for this case. Thanks!

Comment 19

[Rachelle Yang [:ryang][[e-mail]]]

Hi Eric, thank you so much for your great support !

Comment 20

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

(In reply to Eric Chou [:ericchou] [:echou] from comment #18)
> (In reply to Dave Hylands [:dhylands] from comment #17)
> > Comment on attachment 8506078 [details] [diff] [review]
> > patch 1: v1: Use a temporary (.part) file to store incoming file
> > 
> > Review of attachment 8506078 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/bluetooth/bluedroid/BluetoothOppManager.cpp
> > @@ +603,5 @@
> > > +  path.Append(mFileName);
> > > +  path.AppendLiteral(".part");
> > > +
> > > +  mDsFile =
> > > +    DeviceStorageFile::CreateUnique(path, nsIFile::NORMAL_FILE_TYPE, 0644);
> > 
> > I think that this causes mDsFile to get the name with the .part extension.
> > 
> > Then later in NotifyAboutFileChange, I think that it will still have the
> > .part extension.
> > 
> 
> The file name of mDsFile would be renamed to mFileName before
> NotifyAboutFileChange was called, so the file name which was embedded in the
> notification shouldn't contain the .part extension.
> 

I just realized changing file name of mDsFile->mFile is enough for firing correct name to Gaia. NotifyAboutFileChange notifies Gaia about the file change with using mDsFile->mPath instead of mDsFile->mFile->path. So except Rename I also need to reset the path.

Comment 21

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

Attachment: patch 1: v2: Use a temporary (.part) file to store incoming file

Comment 22

[Ben Tian (inactive)]

Comment on attachment 8507785 [details] [diff] [review]
patch 1: v2: Use a temporary (.part) file to store incoming file

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

Please see my comment below.

::: dom/bluetooth/bluedroid/BluetoothOppManager.cpp
@@ +574,5 @@
>  bool
>  BluetoothOppManager::CreateFile()
>  {
>    MOZ_ASSERT(mPutPacketReceivedLength == mPacketLength);
>  

Leave comment at the beginning to state filename and usage of the 2 created files, and explain why we need them.

@@ +596,5 @@
> +
> +  // Prepare the entire file path for the .part file
> +  path.Truncate();
> +  path.AssignLiteral(TARGET_SUBDIR);
> +  path.Append(mFileName);

How about |mTempDsFile->mFile->GetPath(path)| to obtain full path directly?

@@ +806,5 @@
>    return (mPutPacketReceivedLength == mPacketLength);
>  }
>  
>  void
> +BluetoothOppManager::RecoverFileName()

Move the two new functions to near |DeleteReceivedFile| in order to group file related functions.

::: dom/bluetooth/bluedroid/BluetoothOppManager.h
@@ +98,5 @@
>    void DiscardBlobsToSend();
>    bool ProcessNextBatch();
>    void ConnectInternal(const nsAString& aDeviceAddress);
> +  void RecoverFileName();
> +  void RemoveTempDsFile();

Rename to |DeleteDummyFile| to conform with |DeleteReceivedFile|. Also move the two functions after |DeleteReceivedFile| to group file related functions.

@@ +204,5 @@
>    nsCOMPtr<nsIOutputStream> mOutputStream;
>    nsCOMPtr<nsIInputStream> mInputStream;
>    nsCOMPtr<nsIVolumeMountLock> mMountLock;
>    nsRefPtr<DeviceStorageFile> mDsFile;
> +  nsRefPtr<DeviceStorageFile> mTempDsFile;

I suggest to rename to |mDummyDsFile| to be clear it's dummy and |mDsFile| is the final one to use.

Comment 23

[Ben Tian (inactive)]

Re-assign to Bruce Sun to follow this bug.

Comment 24

[Rachelle Yang [:ryang][[e-mail]]]

Hi Bruce, 

May you please kindly share any update for us ?

Thank you very much !

Comment 25

[Bruce Sun [:brsun]]

Hi Rachelle,

I am investigating attachment 8507785 [details] [diff] [review] and trying to refine it based on comment 22.

Comment 26

[Rachelle Yang [:ryang][[e-mail]]]

Hi Bruce, Thank you very much for your kindly sharing. 

Thanks!

Comment 27

[Rachelle Yang [:ryang][[e-mail]]]

[Blocking Requested - why for this release]:
Hi Bruce, please kindly share your update.

Comment 28

[Bruce Sun [:brsun]]

(In reply to Ben Tian [:btian] from comment #22)
> @@ +596,5 @@
> > +
> > +  // Prepare the entire file path for the .part file
> > +  path.Truncate();
> > +  path.AssignLiteral(TARGET_SUBDIR);
> > +  path.Append(mFileName);
> 
> How about |mTempDsFile->mFile->GetPath(path)| to obtain full path directly?

Using the result of nsIFile->GetPath() in this case seems not work.

Here is one example:
 - Target file name: VID_0001.3gp
 - Target file path: Download/Bluetooth/VID_0001.3gp
 - Real file name by GetLeafName(): VID_0001-1.3gp
 - Real file path by GetPath(): /storage/sdcard/Download/Bluetooth/VID_0001-1.3gp

If we use "/storage/..." in DeviceStorageFile::CreateUnique(), aOutStorageName of nsDOMDeviceStorage::ParseFullPath() will be "storage", and DeviceStorageFile cannot create a proper file due to nsIVolumeService::GetVolumeByName() failure.

One error message of this symptom could be noticed from logcat:
I Gecko   : ##### DeviceStorage: GetVolumeByName('storage') failed

Comment 29

[Dave Hylands [:dhylands]]

DeviceStorage expects the filenames to be of the form /STORAGE_NAME/relative-path

so /sdcard/Download/Bluetooth/VID_0001-1.3gp would be the correct path to pass to device storage.

Comment 30

[Bruce Sun [:brsun]]

Attachment: bug1079649_bluetooth_opp_partial_file.patch

This patch is based on attachment 8507785 [details] [diff] [review] with the refinement as comment 22.

Comment 31

[Bruce Sun [:brsun]]

(In reply to Dave Hylands [:dhylands] from comment #29)
> DeviceStorage expects the filenames to be of the form
> /STORAGE_NAME/relative-path
> 
> so /sdcard/Download/Bluetooth/VID_0001-1.3gp would be the correct path to
> pass to device storage.

dhylands: Thanks for kind clarification. Just wondering if there is any way or not to convert the path got from nsIFile::GetPath() to an acceptable path for DeviceStorage.

Comment 32

[Rachelle Yang [:ryang][[e-mail]]]

confirmed with partner, launch blocker. set it to 2.0+

Comment 33

[Ben Tian (inactive)]

Comment on attachment 8515804 [details] [diff] [review]
bug1079649_bluetooth_opp_partial_file.patch

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

Please see my comment below.

::: dom/bluetooth/bluedroid/BluetoothOppManager.cpp
@@ +31,5 @@
>  #include "nsNetUtil.h"
>  #include "nsServiceManagerUtils.h"
>  
>  #define TARGET_SUBDIR "Download/Bluetooth/"
> +#define UNCOMPLETED_FILE_EXTENSION ".part"

nit: I suggest to use INCOMPLETE_FILE_EXTENSION.

@@ +174,5 @@
>  private:
>    nsRefPtr<BluetoothSocket> mSocket;
>  };
>  
> +class mozilla::dom::bluetooth::FileWritter

FileWri't'er. No double 't'.

@@ +234,5 @@
> +  path.AssignLiteral(TARGET_SUBDIR);
> +  path.Append(aFileName);
> +
> +  // Create one dummy file to be a placeholder for the target file name.
> +  mPlaceholderFile = DeviceStorageFile::CreateUnique(path, nsIFile::NORMAL_FILE_TYPE, 0644);

nit: this line is too long (> 80 characters).

@@ +241,5 @@
> +  // DeviceStorageFile::CreateUnique() will try to use a different file name
> +  // if the original file name is already occupied by an existing file. So we
> +  // have to get the real file name that DeviceStorageFile chooses.
> +  MOZ_ASSERT(mPlaceholderFile->mFile);
> +  mPlaceholderFile->mFile->GetLeafName(mFileName);

Can we pass |aFileName| here directly? So that |mFileWriter| doesn't have to store |mFileName| for OPP manager.

@@ +248,5 @@
> +  path.AssignLiteral(TARGET_SUBDIR);
> +  path.Append(aFileName);
> +  path.AppendLiteral(UNCOMPLETED_FILE_EXTENSION);
> +
> +  // Create one file with an meaningless file extension to write the received

nit: 'a' meaningless

@@ +249,5 @@
> +  path.Append(aFileName);
> +  path.AppendLiteral(UNCOMPLETED_FILE_EXTENSION);
> +
> +  // Create one file with an meaningless file extension to write the received
> +  // data. By doing this, we can avoid applications from parsing uncompleted

nit: 'prevent' seems more suitable here.

@@ +251,5 @@
> +
> +  // Create one file with an meaningless file extension to write the received
> +  // data. By doing this, we can avoid applications from parsing uncompleted
> +  // data in the middle of the receiving process.
> +  mContentFile = DeviceStorageFile::CreateUnique(path, nsIFile::NORMAL_FILE_TYPE, 0644);

Ditto.

@@ +254,5 @@
> +  // data in the middle of the receiving process.
> +  mContentFile = DeviceStorageFile::CreateUnique(path, nsIFile::NORMAL_FILE_TYPE, 0644);
> +  NS_ENSURE_TRUE(mContentFile, false);
> +
> +  NS_NewLocalFileOutputStream(getter_AddRefs(mOutputStream), mContentFile->mFile);

Ditto.

@@ +281,5 @@
> +    return;
> +  }
> +
> +  if (mOutputStream) {
> +    mOutputStream->Close();

Should we set |mOutputStream| as nullptr here?

@@ +294,5 @@
> +    path.Append(name);
> +    // Delete the placeholder file.
> +    mPlaceholderFile->mFile->Remove(false);
> +  } else {
> +    path.Append(mFileName);

When do we fall into the else condition?

@@ +301,5 @@
> +  if (mContentFile && mContentFile->mFile) {
> +    if (mCompleted) {
> +      MOZ_ASSERT(!name.IsEmpty());
> +      MOZ_ASSERT(mFileName.Equals(name));
> +      // Rename the content file with the target name.

nit: newline before this comment.

@@ +744,2 @@
>  
> +  if (res) {

Remove the |if| since |res| has been ensured to be true.

@@ +758,2 @@
>  
> +  MOZ_ASSERT(mFileWritter->IsFileValie());

Where is this function?

::: dom/bluetooth/bluedroid/BluetoothOppManager.h
@@ +30,5 @@
>  class BluetoothSocket;
>  class ObexHeaderSet;
>  class SendFileBatch;
>  
> +class FileWritter;

nit: alphabetical order.

Comment 34

[Bruce Sun [:brsun]]

Attachment: bug1079649_bluetooth_opp_partial_file.v2.patch

Update the patch as recommended on comment 33 with one exception:
 - FileWriter::mFileName is still kept. This variable is used to store the feasible target name that DeviceStorageFile::CreateUnique() chooses. Make aFileName as an in-out parameter of FileWriter::CreateFile() might tend to confuse the behavior of this function. And it would be reasonable to keep a getter function like GetFileName() in FileWriter to get the target name at any time. So I am still using this variable to cache a name in FileWriter.

Comment 35

[Ben Tian (inactive)]

Comment on attachment 8517287 [details] [diff] [review]
bug1079649_bluetooth_opp_partial_file.v2.patch

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

I'm still unconvinced that |FileWriter| is necessary and think it increases code complexity. I prefer to simplify as Eric's patch. Please let me know for any concern.

Comment 36

[Bruce Sun [:brsun]]

Attachment: bug1079649_bluetooth_opp_partial_file.v3.patch

As discussed, let's not wrap the file replacement logic into another class and just adapt Eric's patch for simplicity.

Comment 37

[Ben Tian (inactive)]

Comment on attachment 8518076 [details] [diff] [review]
bug1079649_bluetooth_opp_partial_file.v3.patch

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

r=me with nits addressed. Please apply this change to OPP managers of bluez and bluedroid under dom/bluetooth and dom/bluetooth2 (4 in total). Thanks.

::: dom/bluetooth/bluedroid/BluetoothOppManager.cpp
@@ +605,5 @@
>  bool
>  BluetoothOppManager::CreateFile()
>  {
> +  // Create one dummy file to be a placeholder for the target file name, and
> +  // create one file with an meaningless file extension to write the received

nit: another file with a meaningless ...

@@ +606,5 @@
>  BluetoothOppManager::CreateFile()
>  {
> +  // Create one dummy file to be a placeholder for the target file name, and
> +  // create one file with an meaningless file extension to write the received
> +  // data. By doing this, we can avoid applications from parsing uncompleted

nit: prevent applications from parsing incomplete ...

@@ +607,5 @@
>  {
> +  // Create one dummy file to be a placeholder for the target file name, and
> +  // create one file with an meaningless file extension to write the received
> +  // data. By doing this, we can avoid applications from parsing uncompleted
> +  // data in the middle of the receiving process.

nit: I suggest to move this comment right before |nsString path;|.

@@ +614,5 @@
>    nsString path;
>    path.AssignLiteral(TARGET_SUBDIR);
>    path.Append(mFileName);
>  
> +  // Use an empty temp file object to occupy the file name, so that after the

nit: empty dummy file ...

Comment 38

[Bruce Sun [:brsun]]

(In reply to Ben Tian [:btian] from comment #37)
> Comment on attachment 8518076 [details] [diff] [review]
> bug1079649_bluetooth_opp_partial_file.v3.patch
> 
> Review of attachment 8518076 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with nits addressed. Please apply this change to OPP managers of bluez
> and bluedroid under dom/bluetooth and dom/bluetooth2 (4 in total). Thanks.

Thanks. Since code changes are not big, I will adapt the same modifications under those 4 folders.

Just curious, do we have any plan to extract common logics of bluetooth-related codes into one single place? Otherwise, this kind of duplication efforts would keep going on and on. I am not sure whether DRY or WET principle is more suitable in our cases.

Comment 39

[Ben Tian (inactive)]

(In reply to Bruce Sun [:brsun] from comment #38)
> Just curious, do we have any plan to extract common logics of
> bluetooth-related codes into one single place? Otherwise, this kind of
> duplication efforts would keep going on and on. I am not sure whether DRY or
> WET principle is more suitable in our cases.

For bluetooth and bluetooth2, we should maintain both until bluetooth2 is online (since 2.2) because dom/bluetooth is still in use.

For bluez and bluedroid, of course we should extract common logics but it's sill undone. Another way is to temporarily stop updating bluez version and adopts bluedroid version once Thomas finishes bluetooth daemon compatible for all underlying bluetooth stacks.

Any thoughts?

Comment 40

[Bruce Sun [:brsun]]

Attachment: bug1079649_placeholder_file.bluetooth.bluedroid.checkin.patch

Comment 41

[Bruce Sun [:brsun]]

Attachment: bug1079649_placeholder_file.bluetooth.bluez.checkin.patch

Comment 42

[Bruce Sun [:brsun]]

Attachment: bug1079649_placeholder_file.bluetooth2.bluedroid.checkin.patch

Comment 43

[Bruce Sun [:brsun]]

Attachment: bug1079649_placeholder_file.bluetooth2.bluez.checkin.patch

Comment 44

[Bruce Sun [:brsun]]

The result on TBPL seems good: https://tbpl.mozilla.org/?tree=Try&rev=5a8de62e0b41

Comment 45

[Ben Tian (inactive)]

https://hg.mozilla.org/integration/b2g-inbound/rev/31105e475a1e
https://hg.mozilla.org/integration/b2g-inbound/rev/5ec0e9324c63
https://hg.mozilla.org/integration/b2g-inbound/rev/4dd6e0b0a0b9
https://hg.mozilla.org/integration/b2g-inbound/rev/c59a60c34b12

Please also request approval‑mozilla‑b2g32 for v2.0 branch uplift.

Comment 46

[Bruce Sun [:brsun]]

Comment on attachment 8520378 [details] [diff] [review]
bug1079649_placeholder_file.bluetooth.bluedroid.checkin.patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 107964
User impact if declined: incomplete receiving data could be parsed by applications, which would result unexpected behaviour.
Testing completed: https://tbpl.mozilla.org/?tree=Try&rev=a429709dcdd6
Risk to taking this patch (and alternatives if risky): little overhead due to one additional file handle would be created per incoming file only during the receiving process.
String or UUID changes made by this patch: N/A

Comment 47

[Bruce Sun [:brsun]]

Comment on attachment 8520379 [details] [diff] [review]
bug1079649_placeholder_file.bluetooth.bluez.checkin.patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 107964
User impact if declined: incomplete receiving data could be parsed by applications, which would result unexpected behaviour.
Testing completed: https://tbpl.mozilla.org/?tree=Try&rev=a429709dcdd6
Risk to taking this patch (and alternatives if risky): little overhead due to one additional file handle would be created per incoming file only during the receiving process.
String or UUID changes made by this patch: N/A

Comment 48

[Bruce Sun [:brsun]]

Comment on attachment 8520380 [details] [diff] [review]
bug1079649_placeholder_file.bluetooth2.bluedroid.checkin.patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 107964
User impact if declined: incomplete receiving data could be parsed by applications, which would result unexpected behaviour.
Testing completed: https://tbpl.mozilla.org/?tree=Try&rev=a429709dcdd6
Risk to taking this patch (and alternatives if risky): little overhead due to one additional file handle would be created per incoming file only during the receiving process.
String or UUID changes made by this patch: N/A

Comment 49

[Bruce Sun [:brsun]]

Comment on attachment 8520382 [details] [diff] [review]
bug1079649_placeholder_file.bluetooth2.bluez.checkin.patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 107964
User impact if declined: incomplete receiving data could be parsed by applications, which would result unexpected behaviour.
Testing completed: https://tbpl.mozilla.org/?tree=Try&rev=a429709dcdd6
Risk to taking this patch (and alternatives if risky): little overhead due to one additional file handle would be created per incoming file only during the receiving process.
String or UUID changes made by this patch: N/A

Comment 50

[Bruce Sun [:brsun]]

The result of 2.0 TPBL: https://tbpl.mozilla.org/?tree=Try&rev=a429709dcdd6

The red results (Gij, Cpp) seem not caused by these patches, and those tests do not run on the current 2.0 TPBL tree: https://tbpl.mozilla.org/?tree=Mozilla-B2g32-v2.0

Comment 51

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/31105e475a1e
https://hg.mozilla.org/mozilla-central/rev/5ec0e9324c63
https://hg.mozilla.org/mozilla-central/rev/4dd6e0b0a0b9
https://hg.mozilla.org/mozilla-central/rev/c59a60c34b12

Comment 52

[Ryan VanderMeulen [:RyanVM]]

Does this need to land on b2g34 (v2.1) as well? Needs approval requests for that as well if so :)

Comment 53

[Bruce Sun [:brsun]]

Comment on attachment 8520378 [details] [diff] [review]
bug1079649_placeholder_file.bluetooth.bluedroid.checkin.patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 107964
User impact if declined: incomplete receiving data could be parsed by applications, which would result unexpected behaviour.
Testing completed: https://tbpl.mozilla.org/?tree=Try&rev=51fa60d69cb4
Risk to taking this patch (and alternatives if risky): little overhead due to one additional file handle would be created per incoming file only during the receiving process.
String or UUID changes made by this patch: N/A

Comment 54

[Bruce Sun [:brsun]]

Comment on attachment 8520379 [details] [diff] [review]
bug1079649_placeholder_file.bluetooth.bluez.checkin.patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 107964
User impact if declined: incomplete receiving data could be parsed by applications, which would result unexpected behaviour.
Testing completed: https://tbpl.mozilla.org/?tree=Try&rev=51fa60d69cb4
Risk to taking this patch (and alternatives if risky): little overhead due to one additional file handle would be created per incoming file only during the receiving process.
String or UUID changes made by this patch: N/A

Comment 55

[Bruce Sun [:brsun]]

Comment on attachment 8520380 [details] [diff] [review]
bug1079649_placeholder_file.bluetooth2.bluedroid.checkin.patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 107964
User impact if declined: incomplete receiving data could be parsed by applications, which would result unexpected behaviour.
Testing completed: https://tbpl.mozilla.org/?tree=Try&rev=51fa60d69cb4
Risk to taking this patch (and alternatives if risky): little overhead due to one additional file handle would be created per incoming file only during the receiving process.
String or UUID changes made by this patch: N/A

Comment 56

[Bruce Sun [:brsun]]

Comment on attachment 8520382 [details] [diff] [review]
bug1079649_placeholder_file.bluetooth2.bluez.checkin.patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 107964
User impact if declined: incomplete receiving data could be parsed by applications, which would result unexpected behaviour.
Testing completed: https://tbpl.mozilla.org/?tree=Try&rev=51fa60d69cb4
Risk to taking this patch (and alternatives if risky): little overhead due to one additional file handle would be created per incoming file only during the receiving process.
String or UUID changes made by this patch: N/A

Comment 57

[bhavana bajaj [:bajaj]]

This patch seems rather risky to me at this point, Rachelle I think we should reconsider the blocking decision here by confirming with the partner that this is indeed a launch blocker giving them insight into the risk here. Can you please help discuss this again ?

Comment 58

[Rachelle Yang [:ryang][[e-mail]]]

Hi Bhavana,thanks! 
Partner could cherry pick this patch.Thank you very much!

Comment 59

[bhavana bajaj [:bajaj]]

(In reply to Rachelle Yang [:ryang][[e-mail]] from comment #58)
> Hi Bhavana,thanks! 
> Partner could cherry pick this patch.Thank you very much!

Given this going to be cherry picked anyways, I want to avoid the unfair advantage and uplift this for all partners ;) That way we can also avoid regressing the same issue for this partner in 2.1.

Bruce, can you please work with Brain Huang to get targeted testing/verification here once this lands on branches? Thanks!

Comment 60

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

b2g34_v2_1:
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/db4bba7c9655
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/6d04a30b2ae4
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/faca76827802
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/fe058937863d

b2g32_v2_0:
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/cc9648b90a05
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/5eb11fec38d8
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/1c9e3d36ecf3
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/cce5c0601d59

Comment 61

[Kai-Zhen Li [:seinlin][:kli]]

http://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/cce5c0601d59
http://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/1c9e3d36ecf3
http://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/5eb11fec38d8
http://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/cc9648b90a05

Comment 62

[Brian Huang [:brianhuang][:brhuang]]

Hi Mike,

Could you check this and verify it? Please check this with Bruce.

Thank you,
Brian

Comment 63

[Mike Lien[:mlien]]

verified and fixed on v2.0, v2.1, and v2.2

v2.0
Gaia-Rev        124c2f85f1b956e9e8429dab5121de702a3bc197
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/fc90ac008cc1
Build-ID        20141123160207
Version         32.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  40
FW-Date         Tue Oct 21 15:59:42 CST 2014
Bootloader      L1TC10011880

v2.1
Gaia-Rev        afdfa629be209dd53a1b7b6d6c95eab7077ffcd9
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/3d2d47bfe3ff
Build-ID        20141123161201
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  40
FW-Date         Tue Oct 21 15:59:42 CST 2014
Bootloader      L1TC10011880

v2.2
Gaia-Rev        c5bad6d78c5fe168e3bb894fc5cb70902c9b19b1
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/8c02f3280d0c
Build-ID        20141123160201
Version         36.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  40
FW-Date         Tue Oct 21 15:59:42 CST 2014
Bootloader      L1TC10011880

There might have a side effect, please keep following bug 1103849

Comment 64

[William Hsu [:whsu]]

Remove "verifyme" tag.
Thanks everyone! :)