Closed Bug 785124 Opened 12 years ago Closed 12 years ago

B2G Updates: Download Gecko updates with better fallbacks

Categories

(Toolkit :: Application Update, defect, P1)

Product:
Toolkit
Component:
Application Update
Platform:
ARM
Gonk (Firefox OS)
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
B2G C4 (2jan on)
Project Flags:
blocking-basecamp +
Tracking Flags:
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 --- fixed

People

(Reporter: marshall, Assigned: dhylands)

References

(
URL
)

Details

Attachments

(8 files, 7 obsolete files)

Part 2 - Js changes to the updater
19.30 KB, patch
marshall
: review+
Details | Diff | Splinter Review
Part 1 - (v2) Add a VolumeMountLock object
58.55 KB, patch
dougt
: review+
Details | Diff | Splinter Review
Part 4 - Fix refs and ptrs (change type &var to type& var)
52.76 KB, patch
dougt
: review+
Details | Diff | Splinter Review
Part 3 (v4) - Modify the updater to support update.link files
5.10 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
Part 5 - Add the missing ASSERTs that Doug wanted
7.96 KB, patch
dougt
: review+
Details | Diff | Splinter Review
Part 6 - Fix JS code that was failing xpcshell tests.
3.60 KB, patch
marshall
: review+
Details | Diff | Splinter Review
Fix nsIncrementalDownload to work on sdcard for B2G
1.64 KB, patch
jduell.mcbugs
: review+
Details | Diff | Splinter Review
Fix test_0030_general.js to work when downloaded file goes to sdcard.
3.74 KB, patch
jst
: review+
Details | Diff | Splinter Review
Downloading updates for B2G will need to follow this basic flow: - When an available update is found, make sure there is enough space on /sdcard (external storage) - When there isn't enough space, or the sdcard isn't available, check space on /data/local - If neither has enough space, show the user a prompt asking them to clear up space, and try again in 30 minutes - Once a location with enough space has been found, start the download - If the disk runs out of space during the download: -- If we were downloading to /sdcard, we can check space on /data/local, and if there is enough space, move the partial file there, and resume the download. -- Otherwise, prompt the user to clear up space, and try again in 30 minutes
blocking-basecamp: --- → ?
Looks like this nom was missed. We will need this logic in v1.. +cjones overholt
Priority: -- → P1
Blocking based on comment #1 and an IRC conversation with Marshall regarding how implementing this follows the UX spec, etc.
blocking-basecamp: ? → +
Attached patch WIP - v1 (obsolete) — — Splinter Review
Posting this WIP for cjones
Will see how much further I can push this along.
Assignee: marshall → jones.chris.g
Assignee: jones.chris.g → fabrice
Dave, can you take this?
Assignee: fabrice → dhylands
Fabrice - I'll give it a go
I hit some updater issues today, and marshall_law suggested I mention them here, since hopefully this bug should resolve them. I was being offered the second (~10 MB) OTA update that was posted today. When I accepted the prompt, my phone would download the full update, but then not bother to apply it. (It stopped before the standard "[ ======= ]" progress bar that normally shows up in logcat output) I never got an "Install update" prompt. It turned out to be that the updater relies on sdcard access, and my phone was connected to my laptop w/ mass storage enabled -- so, the sdcard was unavailable to the updater. I was able to successfully get the update by disabling mass storage mode, rebooting, and trying again.
That should be handled by B2G directly instead of application update by returning an appropriate value for "UpdRootD".
(In reply to Robert Strong [:rstrong] (do not email) from comment #9) > That should be handled by B2G directly instead of application update by > returning an appropriate value for "UpdRootD". Agreed. Dave has plans to lock down the sdcard for the updater while an update is being downloaded / applied, and I wanted to make sure we logged that use case here, since the patch is still a WIP.
Depends on: 806363, 814822
This adds the VolumeMountLock class, and associated support, which while the instance of the class exists, will prevent a mounted volume from being shared with the PC.
Attachment #685950 - Flags: feedback?(doug.turner)
And oh yeah - there's a big comment block in Volume.cpp that explains the overall design of what this change is doing.
ETA please? last activity was about a week ago!
Not sure on the ETA. The part that I'm most familiar with is done and awaiting feedback from Doug Turner. I'm now working on the JS part and I've got the JS attached to the patch working (it needed some changes).
Comment on attachment 685950 [details] [diff] [review] WIP - Add ability to lock a mount Review of attachment 685950 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/ContentChild.cpp @@ +1094,5 @@ > + // Remove warnings about unused arguments > + (void)aFsName; > + (void)aName; > + (void)aState; > + (void)aMountGeneration; does this work everywhere? not sure about some of the crap compilers we use. ::: dom/system/gonk/Volume.cpp @@ +98,3 @@ > > void > Volume::SetState(Volume::STATE aNewState) This only can happen on one thread, right? It might be good to add an assertion. ::: dom/system/gonk/Volume.h @@ +42,5 @@ > // (i.e. path that leads to the files stored on the volume). > const nsCString &MountPoint() const { return mMountPoint; } > > + int32_t MountGeneration() const { return mMountGeneration; } > + bool IsMountLocked() const { return mMountLocked; } just once space between the return type and the method name. @@ +74,5 @@ > void StartCommand(VolumeCommand *aCommand); > > void HandleVoldResponse(int aResponseCode, nsCWhitespaceTokenizer &aTokenizer); > > + static void UpdateMountLock(const nsACString &aVolumeName, trailing ws ::: dom/system/gonk/nsVolume.cpp @@ +140,5 @@ > +nsVolume::UpdateMountLock(const nsAString &aMountLockState) > +{ > + // There are 3 states, unlocked, locked-background, and locked-foreground > + // I figured it was easier to use neagtive logic and compare for unlocked. > + UpdateMountLock(!aMountLockState.Equals(NS_LITERAL_STRING("unlocked"))); EqualsLiteral() ? ::: dom/system/gonk/nsVolumeService.cpp @@ +43,5 @@ > +public: > + NS_DECL_ISUPPORTS > + > + WakeLockCallback(nsVolumeService *aVolumeService) > + :mVolumeService(aVolumeService) Space after the : @@ +72,5 @@ > + } > +private: > + // We're destructed by the nsVolumeService, so we use a bare pointer here > + // to prevent cyclic references. > + nsVolumeService *mVolumeService; Raw pointers are dangerous. I have written a bunch of comments like that and usually end up being assigned topcrash bug a few months later. Much rather you hold a strong reference and add a Shutdown() method and have the nsVolumeService call it. Another approach -- which may work -- is just to have the nsVolumeService implement this listener. @@ +110,5 @@ > FROM_HERE, > NewRunnableFunction(ShutdownVolumeServiceIOThread)); > } > > +/* void BroadcastVolume(nsString volumeName) */ useless comment. @@ +125,5 @@ > + nsCOMPtr<nsIObserverService> obs = GetObserverService(); > + NS_ENSURE_TRUE(obs, NS_NOINTERFACE); > + > + DBG("nsVolumeService::BroadcastVolume for '%s'", vol->NameStr()); > + nsString stateStr(NS_ConvertUTF8toUTF16(vol->StateStr())); I think you want CopyUTF8toUTF16 @@ +194,5 @@ > + nsRefPtr<nsVolume> vol = mVolumeArray[volIndex]; > + nsString mountLockName; > + vol->GetMountLockName(mountLockName); > + if (mountLockName.Equals(aMountLockName)) { > + vol->UpdateMountLock(aMountLockState); can't we return early here? @@ +274,5 @@ > > + // Use CID rather than CONTRACTID so that we can cast the returned pointer > + // to an nsVolumeService > + NS_DEFINE_CID(volumeServiceCID, NS_VOLUMESERVICE_CID); > + nsCOMPtr<nsVolumeService> vs(do_GetService(volumeServiceCID)); I would really rather you not have to do this. Could you instead, create a new interface that just has UpdateVolume(...) and then QI to it? Also.. ugh. ::: layout/build/nsLayoutModule.cpp @@ +325,5 @@ > > #ifdef MOZ_WIDGET_GONK > NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR(nsIGeolocationProvider, > GonkGPSGeolocationProvider::GetSingleton) > +NS_GENERIC_FACTORY_CONSTRUCTOR(nsVolumeService) // after nsIPowerManagerService I don't understand your comment.
Attachment #685950 - Flags: feedback?(doug.turner) → feedback+
(In reply to Doug Turner (:dougt) from comment #15) > ::: dom/ipc/ContentChild.cpp > @@ +1094,5 @@ > > + // Remove warnings about unused arguments > > + (void)aFsName; > > + (void)aName; > > + (void)aState; > > + (void)aMountGeneration; > > does this work everywhere? not sure about some of the crap compilers we use. unused << myUnusedVariable; is the recommended way to do this. (not "(void)"), from https://mxr.mozilla.org/mozilla-central/source/xpcom/glue/unused.h You just need to #include "mozilla/unused.h" and "using namespace mozilla;"
Attached patch WIP - Add missing files (obsolete) — — Splinter Review
I realized that the files I submitted for feedback were missing a couple of files. Added in this patchset. These will get merged together before submitting, but I figured I'd get feedback on these as well.
Attachment #688364 - Flags: feedback?(doug.turner)
(In reply to Doug Turner (:dougt) from comment #15) > @@ +72,5 @@ > > + } > > +private: > > + // We're destructed by the nsVolumeService, so we use a bare pointer here > > + // to prevent cyclic references. > > + nsVolumeService *mVolumeService; > > Raw pointers are dangerous. I have written a bunch of comments like that > and usually end up being assigned topcrash bug a few months later. > > Much rather you hold a strong reference and add a Shutdown() method and have > the nsVolumeService call it. So that presumably implies listening for xpcom-shutdown and calling Shutdown at that time? > Another approach -- which may work -- is just to have the nsVolumeService > implement this listener. I'll see if I can do that. > @@ +110,5 @@ > > FROM_HERE, > > NewRunnableFunction(ShutdownVolumeServiceIOThread)); > > } > > > > +/* void BroadcastVolume(nsString volumeName) */ > > useless comment. Perhaps. But that's what the IDL code generator puts as the suggested code style to use. If it's really all that useless then shouldn't the code generator be fixed? > @@ +125,5 @@ > > + nsCOMPtr<nsIObserverService> obs = GetObserverService(); > > + NS_ENSURE_TRUE(obs, NS_NOINTERFACE); > > + > > + DBG("nsVolumeService::BroadcastVolume for '%s'", vol->NameStr()); > > + nsString stateStr(NS_ConvertUTF8toUTF16(vol->StateStr())); > > I think you want CopyUTF8toUTF16 So what's the difference? NS_ConvertUTF8toUTF16 and ConvertUTF8toUTF16 both call NS_CStringtoUTF16 What I'm getting at, is this just a preferred convention? When should I use CopyUTF8toUTF16 versus using NS_ConvertUTF8toUTF16? I'm just trying to learn here, not justify. > @@ +274,5 @@ > > > > + // Use CID rather than CONTRACTID so that we can cast the returned pointer > > + // to an nsVolumeService > > + NS_DEFINE_CID(volumeServiceCID, NS_VOLUMESERVICE_CID); > > + nsCOMPtr<nsVolumeService> vs(do_GetService(volumeServiceCID)); > > I would really rather you not have to do this. Could you instead, create a > new interface that just has UpdateVolume(...) and then QI to it? Also.. ugh. I can add UpdateVolume to the IDL. It's just that it's an internal function and not something I want other users of the VolumeService to call. > ::: layout/build/nsLayoutModule.cpp > @@ +325,5 @@ > > > > #ifdef MOZ_WIDGET_GONK > > NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR(nsIGeolocationProvider, > > GonkGPSGeolocationProvider::GetSingleton) > > +NS_GENERIC_FACTORY_CONSTRUCTOR(nsVolumeService) // after nsIPowerManagerService > > I don't understand your comment. I need the nsIPowerManagerService to be constructed before nsVolumeService is constructed (since the constructor of nsVolumeService does do_GetService(POWERMANAGERSERVICE_CONTRACTID). The only way that happens is if I move the location in the nsLayoutModule.cpp file.
Depends on: 818131
Target Milestone: --- → B2G C3 (12dec-1jan)
The VolumeMountLock object, when locked, prevents the indicated volume (typically the sdcard) from being shared with the PC.
Attachment #674974 - Attachment is obsolete: true
Attachment #685950 - Attachment is obsolete: true
Attachment #688364 - Attachment is obsolete: true
Attachment #688364 - Flags: feedback?(doug.turner)
Attachment #692514 - Flags: review?(doug.turner)
Modifies the updater to prefer using the sdcard. Also introduces the notion of a link file. The update meta-data is always stored in /data/local/updates/0 on the phone, and the update.link allows the actual patch file to live elsewhere (i.e. on the sdcard)
Attachment #692515 - Flags: review?(marshall)
Modifies the updater to support update.link files. Modifies the UpdateDriver to prefer sdcard for B2G builds
Attachment #692516 - Flags: review?(robert.bugzilla)
I submitted to try and I'm pasting the link before I know how good or bad it will be: https://tbpl.mozilla.org/?tree=Try&rev=136a18bffb5e
Comment on attachment 692516 [details] [diff] [review] Part 3 - Modify the updater to support update.link files >diff --git a/toolkit/xre/nsUpdateDriver.cpp b/toolkit/xre/nsUpdateDriver.cpp >index c6752b7..916a17a 100644 >--- a/toolkit/xre/nsUpdateDriver.cpp >+++ b/toolkit/xre/nsUpdateDriver.cpp >@@ -971,18 +971,27 @@ nsUpdateProcessor::ProcessUpdate(nsIUpdate* aUpdate) > > NS_ENSURE_ARG_POINTER(aUpdate); > > nsAutoCString binPath; > nsXREDirProvider* dirProvider = nsXREDirProvider::GetSingleton(); > if (dirProvider) { // Normal code path > // Check for and process any available updates > bool persistent; >- nsresult rv = dirProvider->GetFile(XRE_UPDATE_ROOT_DIR, &persistent, >- getter_AddRefs(updRoot)); >+ nsresult rv = NS_OK; >+#ifdef MOZ_WIDGET_GONK >+ // Check in the sdcard for updates first, since that's our preferred >+ // download location. >+ rv = dirProvider->GetFile("UpdArchD", &persistent, >+ getter_AddRefs(updRoot)); >+#endif >+ if (NS_FAILED(rv)) { looks like this will never happen for non gonk. could you run these patches through try as well? >+ rv = dirProvider->GetFile(XRE_UPDATE_ROOT_DIR, &persistent, >+ getter_AddRefs(updRoot)); >+ } > // XRE_UPDATE_ROOT_DIR may fail. Fallback to appDir if failed > if (NS_FAILED(rv)) > updRoot = dirProvider->GetAppDir(); > > greDir = dirProvider->GetGREDir(); > appDir = dirProvider->GetAppDir(); > appVersion = gAppData->version; > argc = gRestartArgc; >-- >1.7.9.5 >
(In reply to Robert Strong [:rstrong] (do not email) from comment #23) ...snip... > >+ nsresult rv = NS_OK; > >+#ifdef MOZ_WIDGET_GONK > >+ // Check in the sdcard for updates first, since that's our preferred > >+ // download location. > >+ rv = dirProvider->GetFile("UpdArchD", &persistent, > >+ getter_AddRefs(updRoot)); > >+#endif > >+ if (NS_FAILED(rv)) { > looks like this will never happen for non gonk. Doh. > could you run these patches through try as well? Yep - since some of these changes affect other platforms, I really need to have clean try runs before I can push these.
OK - After a couple of tweaks - (The contents of RecvBroadcastVolume needed an ifdef on MOZ_WIDGET_GONK), and a fix to initialize rv correctly (comment 24), this is the try run: https://tbpl.mozilla.org/?tree=Try&rev=b15721fc3252 My try run was based on revision 116114: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=a0e351bde946
Addresses the initialization of rv in comment 24 Try run including this change is mentioned in comment 25
Attachment #692516 - Attachment is obsolete: true
Attachment #692516 - Flags: review?(robert.bugzilla)
Attachment #692996 - Flags: review?(robert.bugzilla)
Comment on attachment 692514 [details] [diff] [review] Part 1 - Add a VolumeMountLock object Review of attachment 692514 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/ContentChild.h @@ +185,5 @@ > virtual bool RecvFilePathUpdate(const nsString& type, const nsString& path, const nsCString& reason); > + virtual bool RecvFileSystemUpdate(const nsString& aFsName, > + const nsString& aName, > + const int32_t& aState, > + const int32_t &aMountGeneration); space after & and before the parameter name. ::: dom/ipc/ContentParent.cpp @@ +234,1 @@ > } What was this change? oh, splinter.. @@ +1048,5 @@ > return true; > } > > +bool > +ContentParent::RecvBroadcastVolume(const nsString &aVolumeName) Space after '&', not before @@ +1052,5 @@ > +ContentParent::RecvBroadcastVolume(const nsString &aVolumeName) > +{ > + nsresult rv; > + nsCOMPtr<nsIVolumeService> vs = do_GetService(NS_VOLUMESERVICE_CONTRACTID, &rv); > + NS_ENSURE_SUCCESS(rv, true); Just set on vs not being null. ::: dom/ipc/ContentParent.h @@ +308,5 @@ > > virtual bool RecvAudioChannelRegisterType(const AudioChannelType& aType); > virtual bool RecvAudioChannelUnregisterType(const AudioChannelType& aType); > > + virtual bool RecvBroadcastVolume(const nsString &volumeName); Space after &, not before. ::: dom/ipc/PContent.ipdl @@ +326,5 @@ > > FilePathUpdate(nsString type, nsString filepath, nsCString reasons); > > + FileSystemUpdate(nsString fsName, nsString mountPoint, int32_t fsState, > + int32_t mountInstance); Shouldn't this be called 'mountGeneration' not 'mountInstance'? ::: dom/system/gonk/AutoMounter.cpp @@ +406,5 @@ > + (int)vol->IsMountLocked()); > + } else { > + LOG("UpdateState: Volume %s is %s and %s", vol->NameStr(), vol->StateStr(), > + vol->MediaPresent() ? "inserted" : "missing"); > + } Can you wrap this entire block in whatever #define that LOG uses? @@ +421,5 @@ > + // The volume is currently locked, so leave it in the mounted > + // state. > + DBG("UpdateState: Mounted volume %s is locked, leaving", > + vol->NameStr()); > + break; could you use return here? I do not think you need to do anything else int his method, right? (not enough context to see in this patch, but the next block uses return). ::: dom/system/gonk/Volume.cpp @@ +36,5 @@ > +// We eliminate the race conditions by introducing the concept of a > +// generation number. Every time a volume transitions to the Mounted > +// state, it gets assigned a new generation number. Whenever the state > +// of a Volume changes, we send the updated state and current generation > +// number to the main thread where it gets updated in the nsVolume. So, we only update the generation number on the main thread, and we are guarding that with assertions, right? @@ +56,5 @@ > : mMediaPresent(true), > mState(nsIVolume::STATE_INIT), > + mName(aName), > + mMountGeneration(-1), > + mMountLocked(true) // Needs to agree with nsVolume::nsVolume i have seen a different style that is more popular: : mMediaPresent(true) , mState(nsIVolume::STATE_INIT) , ...... @@ +98,4 @@ > > void > Volume::SetState(Volume::STATE aNewState) > { This can only happen on the main thread. Please add an assertion. @@ +191,5 @@ > +void > +Volume::UpdateMountLock(const nsACString &aVolumeName, > + const int32_t &aMountGeneration, > + const bool &aMountLocked) > +{ This also can only happen on the main thread, right? ::: dom/system/gonk/VolumeServiceIOThread.cpp @@ +12,5 @@ > namespace mozilla { > namespace system { > > +VolumeServiceIOThread::VolumeServiceIOThread(nsVolumeService *aVolumeService) > + : mVolumeService(aVolumeService) So the IOThread has a strong reference to the volume service. How do we make sure that we release the volume service -- do we kill this IO thread at shutdown? @@ +62,4 @@ > } > } > > static RefPtr<VolumeServiceIOThread> sVolumeServiceIOThread; shouldn't you be using StaticRefPtr<T> @@ +69,3 @@ > { > MOZ_ASSERT(MessageLoop::current() == XRE_GetIOMessageLoop()); > + sVolumeServiceIOThread = new VolumeServiceIOThread(aVolumeService); Shouldn't you assert that sVolumeServiceIOThread is null before calling new? ::: dom/system/gonk/VolumeServiceIOThread.h @@ +22,5 @@ > public Volume::EventObserver, > public RefCounted<VolumeServiceIOThread> > { > public: > + VolumeServiceIOThread(nsVolumeService *aVolumeService); space between the * and the type ::: dom/system/gonk/nsIVolumeMountLock.idl @@ +7,5 @@ > +[scriptable, uuid(44449f34-5ca1-4aff-bce6-22c79263de24)] > +interface nsIVolumeMountLock : nsISupports > +{ > + void unlock(); > +}; Just move this interface into nsIVolume.idl. Purist might complain, but nsIVolume.idl already has CID's listed in it... so wahtever. ::: dom/system/gonk/nsVolume.cpp @@ +55,5 @@ > return NS_OK; > } > > +NS_IMETHODIMP nsVolume::GetMountGeneration(int32_t *aMountGeneration) > +{ type& aName @@ +60,5 @@ > + *aMountGeneration = mMountGeneration; > + return NS_OK; > +} > + > +NS_IMETHODIMP nsVolume::GetMountLockName(nsAString &aMountLockName) type& aName @@ +104,5 @@ > + LOG("nsVolume: %s state %s", NameStr(), StateStr()); > +} > + > +void nsVolume::Set(const nsVolume *aVolume) > +{ not an operator= ? ::: dom/system/gonk/nsVolume.h @@ +26,5 @@ > nsVolume(const Volume *aVolume); > > + // This constructor is used by ContentChild::RecvFileSystemUpdate > + nsVolume(const nsAString &aName, const nsAString &aMountPoint, > + const int32_t &aState, const int32_t &aMountGeneration) type& aName ::: dom/system/gonk/nsVolumeMountLock.h @@ +28,5 @@ > +{ > +public: > + NS_DECL_ISUPPORTS > + NS_DECL_NSIVOLUMEMOUNTLOCK > + Why aren't you using NS_DECL_NSIOBSERVER instead? @@ +30,5 @@ > + NS_DECL_ISUPPORTS > + NS_DECL_NSIVOLUMEMOUNTLOCK > + > + // Returns a strong reference to the VolumeMountLock instance for the indicated > + // volume. drop this comment. it is more confusing (to me at least) than just the method name. :) ::: dom/system/gonk/nsVolumeService.h @@ +35,4 @@ > > nsVolumeService(); > > + static nsVolumeService *GetSingleton(); I like returning already_AddRefed<> because then users tend not to screw up. @@ +43,4 @@ > already_AddRefed<nsVolume> FindVolumeByName(const nsAString &aName); > already_AddRefed<nsVolume> FindAddVolumeByName(const nsAString &aName); > void UpdateVolume(const nsVolume *aVolume); > + void UpdateVolumeIOThread(const Volume *aVolume); type* aName @@ +49,5 @@ > ~nsVolumeService(); > > + nsVolume::Array mVolumeArray; > + > + static StaticRefPtr<nsVolumeService> sSingleton; one space between the > and s ::: layout/build/nsLayoutModule.cpp @@ +328,5 @@ > NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR(nsIGeolocationProvider, > GonkGPSGeolocationProvider::GetSingleton) > +// Since the nsVolumeService constructor calls into nsIPowerManagerService, > +// we need it to be constructed sometime after nsIPowerManagerService. > +NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR(nsVolumeService, I do not understand this comment. Is that really true? What is the build error if you just leave this alone? ::: layout/build/nsLayoutStatics.cpp @@ +353,3 @@ > nsCORSListenerProxy::Shutdown(); > > nsIPresShell::ReleaseStatics(); There is some trailing white space in this file. Mind cleaning it up while you are here? M-x delete-trailing-whitespace
(In reply to Doug Turner (:dougt) from comment #27) > ::: dom/ipc/ContentParent.cpp > @@ +234,1 @@ > > } > > What was this change? oh, splinter.. Oh that was an indentation change I had done in a previous patch that wasn't consistent with the rest of the file. I noticed it while doing this patch and fixed it. > @@ +1052,5 @@ > > +ContentParent::RecvBroadcastVolume(const nsString &aVolumeName) > > +{ > > + nsresult rv; > > + nsCOMPtr<nsIVolumeService> vs = do_GetService(NS_VOLUMESERVICE_CONTRACTID, &rv); > > + NS_ENSURE_SUCCESS(rv, true); > > Just set on vs not being null. I don't understand this comment. Or did you mean that I should do: if (!vs) { vs->BroadcastVolume(aVolumeName); } return true; > ::: dom/ipc/PContent.ipdl > @@ +326,5 @@ > > > > FilePathUpdate(nsString type, nsString filepath, nsCString reasons); > > > > + FileSystemUpdate(nsString fsName, nsString mountPoint, int32_t fsState, > > + int32_t mountInstance); > > Shouldn't this be called 'mountGeneration' not 'mountInstance'? Yep - I had renamed it along the way and missed that one. > ::: dom/system/gonk/AutoMounter.cpp > @@ +406,5 @@ > > + (int)vol->IsMountLocked()); > > + } else { > > + LOG("UpdateState: Volume %s is %s and %s", vol->NameStr(), vol->StateStr(), > > + vol->MediaPresent() ? "inserted" : "missing"); > > + } > > Can you wrap this entire block in whatever #define that LOG uses? LOG doesn't use a macro (i.e. this LOG is always done) and its quite intentional. It's a very low frequency log statment, once per state change of the volume (which is a few log lines per SD card insertion/removal. > @@ +421,5 @@ > > + // The volume is currently locked, so leave it in the mounted > > + // state. > > + DBG("UpdateState: Mounted volume %s is locked, leaving", > > + vol->NameStr()); > > + break; > > could you use return here? I do not think you need to do anything else int > his method, right? (not enough context to see in this patch, but the next > block uses return). Hmm. For a single volume, either one works. For multiple volumes, this needs to be a break, so the returns are incorrect. > ::: dom/system/gonk/Volume.cpp > @@ +36,5 @@ > > +// We eliminate the race conditions by introducing the concept of a > > +// generation number. Every time a volume transitions to the Mounted > > +// state, it gets assigned a new generation number. Whenever the state > > +// of a Volume changes, we send the updated state and current generation > > +// number to the main thread where it gets updated in the nsVolume. > > So, we only update the generation number on the main thread, and we are > guarding that with assertions, right? I can throw more assertions in. nsVolumeXxx is all main thread, and VolumeXxx is all IOThread. > @@ +56,5 @@ > > : mMediaPresent(true), > > mState(nsIVolume::STATE_INIT), > > + mName(aName), > > + mMountGeneration(-1), > > + mMountLocked(true) // Needs to agree with nsVolume::nsVolume > > i have seen a different style that is more popular: > > : mMediaPresent(true) > , mState(nsIVolume::STATE_INIT) > , ...... Yeah and I cringe every time I see it. > @@ +98,4 @@ > > > > void > > Volume::SetState(Volume::STATE aNewState) > > { > > This can only happen on the main thread. Please add an assertion. Actually, this can ONLY happen on the IOThread. Volume methods are all IOThread (nsVolume methods are all MainThread). SetState is a private method. I chose to put assertions in the API entry points that outsiders can use. If you want I can add an assertion in every single Volume::Xxx function to verify that its on the IOThread and add an assertion in every single nsVolume:Xxx function to verify that its on the MainThread, but that seems like overkill to me. > @@ +191,5 @@ > > +void > > +Volume::UpdateMountLock(const nsACString &aVolumeName, > > + const int32_t &aMountGeneration, > > + const bool &aMountLocked) > > +{ > > This also can only happen on the main thread, right? This is a Volume method, so it should only happen on the IOThread. > ::: dom/system/gonk/VolumeServiceIOThread.cpp > @@ +12,5 @@ > > namespace mozilla { > > namespace system { > > > > +VolumeServiceIOThread::VolumeServiceIOThread(nsVolumeService *aVolumeService) > > + : mVolumeService(aVolumeService) > > So the IOThread has a strong reference to the volume service. How do we > make sure that we release the volume service -- do we kill this IO thread at > shutdown? nsVolumeService::Shutdown posts a call to ShutdownVolumeServiceIOThread > > @@ +62,4 @@ > > } > > } > > > > static RefPtr<VolumeServiceIOThread> sVolumeServiceIOThread; > > shouldn't you be using StaticRefPtr<T> > > @@ +69,3 @@ > > { > > MOZ_ASSERT(MessageLoop::current() == XRE_GetIOMessageLoop()); > > + sVolumeServiceIOThread = new VolumeServiceIOThread(aVolumeService); > > Shouldn't you assert that sVolumeServiceIOThread is null before calling new? > > ::: dom/system/gonk/VolumeServiceIOThread.h > @@ +22,5 @@ > > public Volume::EventObserver, > > public RefCounted<VolumeServiceIOThread> > > { > > public: > > + VolumeServiceIOThread(nsVolumeService *aVolumeService); > > space between the * and the type > > ::: dom/system/gonk/nsIVolumeMountLock.idl > @@ +7,5 @@ > > +[scriptable, uuid(44449f34-5ca1-4aff-bce6-22c79263de24)] > > +interface nsIVolumeMountLock : nsISupports > > +{ > > + void unlock(); > > +}; > > Just move this interface into nsIVolume.idl. Purist might complain, but > nsIVolume.idl already has CID's listed in it... so wahtever. > > ::: dom/system/gonk/nsVolume.cpp > @@ +55,5 @@ > > return NS_OK; > > } > > > > +NS_IMETHODIMP nsVolume::GetMountGeneration(int32_t *aMountGeneration) > > +{ > > type& aName > > @@ +60,5 @@ > > + *aMountGeneration = mMountGeneration; > > + return NS_OK; > > +} > > + > > +NS_IMETHODIMP nsVolume::GetMountLockName(nsAString &aMountLockName) > > type& aName I don't use that convention in these files. The & belongs with the variable name and not the type. i.e. If you do nsAString& a, b; then b is NOT a reference. Coding it like nsAString &a, b; makes it clear that a is an nsString reference and b is an nsString. If I fix this one, I'll need to fix the other 300 occurrences in these files to make them consistent. > @@ +104,5 @@ > > + LOG("nsVolume: %s state %s", NameStr(), StateStr()); > > +} > > + > > +void nsVolume::Set(const nsVolume *aVolume) > > +{ > > not an operator= ? It's called in exactly one place in the code, so I didn't think it warranted an operator = > ::: dom/system/gonk/nsVolumeMountLock.h > @@ +28,5 @@ > > +{ > > +public: > > + NS_DECL_ISUPPORTS > > + NS_DECL_NSIVOLUMEMOUNTLOCK > > + > > Why aren't you using NS_DECL_NSIOBSERVER instead? Technically, I should probably have NS_DECL_NSIOBSERVER as well, and remove the prototype for the Observe method. > > @@ +30,5 @@ > > + NS_DECL_ISUPPORTS > > + NS_DECL_NSIVOLUMEMOUNTLOCK > > + > > + // Returns a strong reference to the VolumeMountLock instance for the indicated > > + // volume. > > drop this comment. it is more confusing (to me at least) than just the > method name. :) Yeah - I was inherting from ISupportsWeakReference so I didn't want people to think it might be returning a weak reference. > ::: dom/system/gonk/nsVolumeService.h > @@ +35,4 @@ > > > > nsVolumeService(); > > > > + static nsVolumeService *GetSingleton(); > > I like returning already_AddRefed<> because then users tend not to screw up. All the examples that use NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR that I could find return a bare pointer. I also verified that using already_AddRefed can be used here, so I can change it. > ::: layout/build/nsLayoutModule.cpp > @@ +328,5 @@ > > NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR(nsIGeolocationProvider, > > GonkGPSGeolocationProvider::GetSingleton) > > +// Since the nsVolumeService constructor calls into nsIPowerManagerService, > > +// we need it to be constructed sometime after nsIPowerManagerService. > > +NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR(nsVolumeService, > > I do not understand this comment. Is that really true? What is the build > error if you just leave this alone? There is no build error. However pmService will be assigned null in the nsVolumeService constructor when I do: nsCOMPtr<nsIPowerManagerService> pmService = do_GetService(POWERMANAGERSERVICE_CONTRACTID); If nsIVolumeService is constructed after nsIPowerManager, then pmService is assigned properly. I talked with Kyle Huey and he said that it was fine to call do_GetService from my constructor as long as there weren't any circular references (and Kyle said that this was done all the time) > ::: layout/build/nsLayoutStatics.cpp > @@ +353,3 @@ > > nsCORSListenerProxy::Shutdown(); > > > > nsIPresShell::ReleaseStatics(); > > There is some trailing white space in this file. Mind cleaning it up while > you are here? M-x delete-trailing-whitespace Sure. I don't use emacs, but I have Edit->Other->Remove Trailing Whitespace :) I actually have a utiltiy I wrote for checking line endings (and I considered trailing whitespace a type of line ending) https://github.com/dhylands/projects/tree/master/utils/src/CountLineEndings
Address review comments
Attachment #692514 - Attachment is obsolete: true
Attachment #692514 - Flags: review?(doug.turner)
Attachment #693195 - Flags: review?(doug.turner)
This patchset should just contain whitespace changes.
Attachment #693196 - Flags: review?(doug.turner)
Comment on attachment 692996 [details] [diff] [review] Part 3 (v2) - Modify the updater to support update.link files Brian, could you take a look at this as well? Thanks
Attachment #692996 - Flags: review?(netzen)
Comment on attachment 692996 [details] [diff] [review] Part 3 (v2) - Modify the updater to support update.link files Review of attachment 692996 [details] [diff] [review]: ----------------------------------------------------------------- Look good to me. I don't see any issues security wise as far as MAR verification goes because we verify the signature after that path is obtained. So even if someone staged a .link file it wouldn't help them. ::: toolkit/mozapps/update/updater/updater.cpp @@ +2074,5 @@ > + *fileName = NS_T('\0'); > + return READ_ERROR; > + } > + dataFileName[bytesRead] = '\0'; > + if ((bytesRead > 0 ) && (dataFileName[bytesRead-1] == '\n')) { do another similar block in addition to this for '\r' Which would cover \n, \r and \r\n @@ +2081,5 @@ > + dataFileName[bytesRead] = '\0'; > + } > + > +#if defined(XP_WIN) > + size_t charsRequired = mbstowcs(NULL, dataFileName, 0); Maybe use MultiByteToWideChar so you have UTF8 support for proper unicode conversion: http://msdn.microsoft.com/en-us/library/windows/desktop/dd319072%28v=vs.85%29.aspx Although I don't think we need this functionality on Windows so I'm fine if you want to keep it as is. ::: toolkit/xre/nsUpdateDriver.cpp @@ +990,5 @@ > + nsresult rv = NS_ERROR_FAILURE; // Take the NS_FAILED path when non-GONK > +#ifdef MOZ_WIDGET_GONK > + // Check in the sdcard for updates first, since that's our preferred > + // download location. > + rv = dirProvider->GetFile("UpdArchD", &persistent, Maybe check with someone else if this is the correct dir key
Attachment #692996 - Flags: review?(netzen) → feedback+
Comment on attachment 693195 [details] [diff] [review] Part 1 - (v2) Add a VolumeMountLock object Review of attachment 693195 [details] [diff] [review]: ----------------------------------------------------------------- I think you should add more thread assertions in Volume::... but not going to block this on that. otherwises lgtm
Attachment #693195 - Flags: review?(doug.turner) → review+
Comment on attachment 693196 [details] [diff] [review] Part 4 - Fix refs and ptrs (change type &var to type& var) consistency is good. rs=me
Attachment #693196 - Flags: review?(doug.turner) → review+
Make changes as per bbondy's review comments (strip trailing CR as well as trailing NL). Also talked with bbondy on IRC and revised GetUpdateFilename so that looking for the update.link file is gonk-only.
Attachment #692996 - Attachment is obsolete: true
Attachment #692996 - Flags: review?(robert.bugzilla)
Comment on attachment 694094 [details] [diff] [review] Part 3 (v3) - Modify the updater to support update.link files So now it looks like all of the changes in this patchset are gonk specific.
Attachment #694094 - Flags: review?(robert.bugzilla)
Added definition of UpdArchD in nsXULAppApi.h and adjusted nsUpdateDriver.cpp to use it.
Attachment #694094 - Attachment is obsolete: true
Attachment #694094 - Flags: review?(robert.bugzilla)
Attachment #694126 - Flags: review?(robert.bugzilla)
Comment on attachment 692515 [details] [diff] [review] Part 2 - Js changes to the updater Review of attachment 692515 [details] [diff] [review]: ----------------------------------------------------------------- Great work! Just some nits :) ::: b2g/components/DirectoryProvider.js @@ +158,5 @@ > + return dir; > + } > + // updates/0 is either a file or isn't writable. In either case we > + // can't use it. > + return null; nit: should we log or when updates/0 isn't a directory / writable here? ::: toolkit/mozapps/update/nsUpdateService.js @@ +982,5 @@ > catch (e) { > LOG("cleanUpUpdatesDir - failed to remove file " + f.path); > } > } > + releaseSDCardMountLock(); should this be surrounded by #ifdef MOZ_WIDGET_GONK? also below on line 2887 and 3236
Attachment #692515 - Flags: review?(marshall) → review+
(In reply to Marshall Culpepper [:marshall_law] from comment #39) > Comment on attachment 692515 [details] [diff] [review] > Part 2 - Js changes to the updater > > Review of attachment 692515 [details] [diff] [review]: > ----------------------------------------------------------------- > > Great work! Just some nits :) > > ::: b2g/components/DirectoryProvider.js > @@ +158,5 @@ > > + return dir; > > + } > > + // updates/0 is either a file or isn't writable. In either case we > > + // can't use it. > > + return null; > > nit: should we log or when updates/0 isn't a directory / writable here? Sure - I can add that. > ::: toolkit/mozapps/update/nsUpdateService.js > @@ +982,5 @@ > > catch (e) { > > LOG("cleanUpUpdatesDir - failed to remove file " + f.path); > > } > > } > > + releaseSDCardMountLock(); > > should this be surrounded by #ifdef MOZ_WIDGET_GONK? also below on line 2887 > and 3236 I put an #ifdef inside the releaseSDCardMountLock function so that we didn't have to clutter up the callers (i.e. releaseSDCardMountLock is an empty function for non-gonk)
I discovered my missing asserts in another source tree (I guess I accidentally edited the wrong file).
Attachment #695056 - Flags: review?(doug.turner)
Comment on attachment 695056 [details] [diff] [review] Part 5 - Add the missing ASSERTs that Doug wanted Review of attachment 695056 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/Volume.cpp @@ +64,5 @@ > > void > Volume::SetMediaPresent(bool aMediaPresent) > { > + MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default)); Extra ')' at the stmt. plz fix them all.
Attachment #695056 - Flags: review?(doug.turner) → review+
(In reply to Doug Turner (:dougt) from comment #42) > Comment on attachment 695056 [details] [diff] [review] > Part 5 - Add the missing ASSERTs that Doug wanted > > Review of attachment 695056 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/system/gonk/Volume.cpp > @@ +64,5 @@ > > > > void > > Volume::SetMediaPresent(bool aMediaPresent) > > { > > + MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default)); > > Extra ')' at the stmt. plz fix them all. I was just testing to see if you were paying attention :) My build failed as soon as I tried it, but I had already put the patch up for review, so I decided to leave it and see if you'd notice.
Whiteboard: [LOE:S] → [LOE:S][needs review robstrong]
Target Milestone: B2G C3 (12dec-1jan) → B2G C4 (2jan on)
Attachment #694126 - Flags: review?(robert.bugzilla) → review+
Need an updated part 5.
Keywords: checkin-needed
Whiteboard: [LOE:S][needs review robstrong] → [LOE:S]
I was planning on landing this later today. I was going to do a little bit of cleanup (based on Marshall's comments). I have the updated patch for part 5 in my tree already
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/95c4247fc5b7 - took a while to see, what with one thing and another, but you were failing 10 update xpcshell emulator tests, a la https://tbpl.mozilla.org/php/getParsedLog.php?id=18580148&tree=Mozilla-Inbound
Blocks: 813770
Version: unspecified → Trunk
So far, I've tried running xpcshell locally, but I can't reproduce the failure on inbound. I also pushed to try again, just the xpcshell test, but it's been sitting in the queue for 16 hours and still hasn't run. https://tbpl.mozilla.org/?tree=Try&rev=e35c980bb797
Whiteboard: [LOE:S] → [underway but landing blocked by 813770]
Attachment #700312 - Flags: review?(jduell.mcbugs)
It turns out that we get different error codes when the mar file gets stored to sdcard versus /data for part 12 of this test. Bug 82858 was filed to investigate this further.
Attachment #700313 - Flags: review?(jst)
Comment on attachment 700312 [details] [diff] [review] Fix nsIncrementalDownload to work on sdcard for B2G Review of attachment 700312 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/src/nsIncrementalDownload.cpp @@ +45,5 @@ > +#if defined(MOZ_WIDGET_GONK) > + // The sdcard on a B2G phone looks like: > + // d---rwx--- system sdcard_rw 1970-01-01 01:00:00 sdcard > + // On the emulator, xpcshell fails when using 0600 mode to open the file, > + // and 0660 works. Get rid of whitespace at end of line here, and in blank line after this.
Attachment #700312 - Flags: review?(jduell.mcbugs) → review+
Attachment #700309 - Flags: review?(marshall) → review+
Comment on attachment 700313 [details] [diff] [review] Fix test_0030_general.js to work when downloaded file goes to sdcard. Looks good, r=jst
Attachment #700313 - Flags: review?(jst) → review+
Whiteboard: [underway but landing blocked by 813770]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: