Closed Bug 1079649 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox OS Graveyard :: Bluetooth, defect, major)

ARM
Gonk (Firefox OS)
defect
Not set
major

Tracking

(blocking-b2g:2.0+, firefox34 wontfix, firefox35 wontfix, firefox36 fixed, b2g-v2.0 verified, b2g-v2.0M fixed, b2g-v2.1 verified, b2g-v2.2 verified)

VERIFIED FIXED
blocking-b2g 2.0+
Tracking Status
firefox34 --- wontfix
firefox35 --- wontfix
firefox36 --- fixed
b2g-v2.0 --- verified
b2g-v2.0M --- fixed
b2g-v2.1 --- verified
b2g-v2.2 --- verified

People

(Reporter: vsireesha246, Assigned: brsun)

References

Details

(Whiteboard: [LibGLA,TD101723,QE2, B])

Attachments

(4 files, 7 obsolete files)

6.64 KB, patch
brsun
: review+
Details | Diff | Splinter Review
6.55 KB, patch
brsun
: review+
Details | Diff | Splinter Review
6.65 KB, patch
brsun
: review+
Details | Diff | Splinter Review
6.56 KB, patch
brsun
: review+
Details | Diff | Splinter Review
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.
Whiteboard: [LibGLA,TD101723,QE2, B]
Attached patch Bug_1079649.patch (obsolete) — Splinter Review
Please find the attached work-around patch for this issue.
Hi Vance Chen,

Would you please help me to review this patch.

Thanks..
Sireesha
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.
(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.
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
Flags: needinfo?(echou)
Flags: needinfo?(dhylands)
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.
Flags: needinfo?(echou)
Attached patch Bug_1079649.patch (obsolete) — Splinter Review
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.
Attachment #8501506 - Attachment is obsolete: true
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?
Flags: needinfo?(echou)
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?)
Flags: needinfo?(dhylands) → needinfo?(dflanagan)
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.
Flags: needinfo?(dflanagan) → needinfo?(dhylands)
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.
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.
Flags: needinfo?(dhylands)
(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.
Assignee: nobody → echou
Flags: needinfo?(echou)
Hi Eric, Could you please kindly share the status of the bug ?
Any comments on the patch in comment7 ?
Thank you very much !!
Flags: needinfo?(echou)
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!
Flags: needinfo?(echou)
Attachment #8506078 - Flags: feedback?(dhylands)
(In reply to Rachelle Yang [:ryang][ryang@mozilla.com] 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.
Flags: needinfo?(ryang)
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.
Attachment #8506078 - Flags: feedback?(dhylands) → feedback+
(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!
Hi Eric, thank you so much for your great support !
Flags: needinfo?(ryang)
(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 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.
Attachment #8507785 - Flags: review?(btian)
Re-assign to Bruce Sun to follow this bug.
Assignee: echou → brsun
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Hi Bruce, 

May you please kindly share any update for us ?

Thank you very much !
Flags: needinfo?(brsun)
Hi Rachelle,

I am investigating attachment 8507785 [details] [diff] [review] and trying to refine it based on comment 22.
Flags: needinfo?(brsun)
Hi Bruce, Thank you very much for your kindly sharing. 

Thanks!
[Blocking Requested - why for this release]:
Hi Bruce, please kindly share your update.
blocking-b2g: --- → 2.0?
Group: kddi-confidential
Group: kddi-confidential
(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
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.
This patch is based on attachment 8507785 [details] [diff] [review] with the refinement as comment 22.
Attachment #8515804 - Flags: review?(btian)
(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.
confirmed with partner, launch blocker. set it to 2.0+
blocking-b2g: 2.0? → 2.0+
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.
Attachment #8515804 - Flags: review?(btian)
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.
Attachment #8515804 - Attachment is obsolete: true
Attachment #8517287 - Flags: review?(btian)
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.
Attachment #8517287 - Flags: review?(btian)
As discussed, let's not wrap the file replacement logic into another class and just adapt Eric's patch for simplicity.
Attachment #8517287 - Attachment is obsolete: true
Attachment #8518076 - Flags: review?(btian)
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 ...
Attachment #8518076 - Flags: review?(btian) → review+
(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.
(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?
Attachment #8501671 - Attachment is obsolete: true
Attachment #8507785 - Attachment is obsolete: true
Attachment #8518076 - Attachment is obsolete: true
Attachment #8520378 - Flags: review+
The result on TBPL seems good: https://tbpl.mozilla.org/?tree=Try&rev=5a8de62e0b41
Keywords: checkin-needed
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
Attachment #8520378 - Flags: approval-mozilla-b2g32?
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
Attachment #8520379 - Flags: approval-mozilla-b2g32?
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
Attachment #8520380 - Flags: approval-mozilla-b2g32?
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
Attachment #8520382 - Flags: approval-mozilla-b2g32?
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
Does this need to land on b2g34 (v2.1) as well? Needs approval requests for that as well if so :)
status-b2g-v2.1: --- → ?
Flags: needinfo?(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
Flags: needinfo?(brsun)
Attachment #8520378 - Flags: approval-mozilla-b2g34?
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
Attachment #8520379 - Flags: approval-mozilla-b2g34?
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
Attachment #8520380 - Flags: approval-mozilla-b2g34?
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
Attachment #8520382 - Flags: approval-mozilla-b2g34?
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 ?
Flags: needinfo?(ryang)
Hi Bhavana,thanks! 
Partner could cherry pick this patch.Thank you very much!
Flags: needinfo?(ryang)
(In reply to Rachelle Yang [:ryang][ryang@mozilla.com] 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!
Flags: needinfo?(brhuang)
Attachment #8520378 - Flags: approval-mozilla-b2g34?
Attachment #8520378 - Flags: approval-mozilla-b2g34+
Attachment #8520378 - Flags: approval-mozilla-b2g32?
Attachment #8520378 - Flags: approval-mozilla-b2g32+
Attachment #8520379 - Flags: approval-mozilla-b2g34?
Attachment #8520379 - Flags: approval-mozilla-b2g34+
Attachment #8520379 - Flags: approval-mozilla-b2g32?
Attachment #8520379 - Flags: approval-mozilla-b2g32+
Attachment #8520380 - Flags: approval-mozilla-b2g34?
Attachment #8520380 - Flags: approval-mozilla-b2g34+
Attachment #8520380 - Flags: approval-mozilla-b2g32?
Attachment #8520380 - Flags: approval-mozilla-b2g32+
Attachment #8520382 - Flags: approval-mozilla-b2g34?
Attachment #8520382 - Flags: approval-mozilla-b2g34+
Attachment #8520382 - Flags: approval-mozilla-b2g32?
Attachment #8520382 - Flags: approval-mozilla-b2g32+
Keywords: verifyme
Hi Mike,

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

Thank you,
Brian
Flags: needinfo?(brhuang) → needinfo?(mlien)
Blocks: 1103849
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
Remove "verifyme" tag.
Thanks everyone! :)
Status: RESOLVED → VERIFIED
Keywords: verifyme
Component: Gaia → Bluetooth
You need to log in before you can comment on or make changes to this bug.