Closed Bug 1203428 Opened 4 years ago Closed 4 years ago

E10S for device storage API

Categories

(B2GDroid Graveyard :: General, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: cyu, Assigned: fabrice)

References

Details

Attachments

(1 file, 2 obsolete files)

The FTU app crashes because AndroidBridge is not available in the content process, while FTU tries to access it:

The crash stack:
(gdb) p sJavaVM
$1 = (JavaVM *) 0x0
(gdb) bt
#0  mozilla::jni::GetEnvForThread () at widget/android/jni/Utils.cpp:110
#1  0x46dc1c68 in mozilla::AndroidBridge::GetExternalPublicDirectory (aType=..., aPath=...) at widget/android/AndroidBridge.cpp:2121
#2  0x46b05ae6 in mozilla::dom::devicestorage::DeviceStorageStatics::InitDirs (this=0x4bb3f040) at dom/devicestorage/DeviceStorageStatics.cpp:137
#3  0x46b05cf2 in mozilla::dom::devicestorage::DeviceStorageStatics::InitializeDirs () at dom/devicestorage/DeviceStorageStatics.cpp:65
#4  0x46b05d1c in DeviceStorageFile::GetRootDirectoryForType (aStorageType=..., aStorageName=..., aFile=0xbeecda58) at dom/devicestorage/nsDeviceStorage.cpp:614
#5  0x46b05e40 in nsDOMDeviceStorage::SetRootDirectoryForType (this=this@entry=0x518d0f90, aStorageType=..., aStorageName=...) at dom/devicestorage/nsDeviceStorage.cpp:1431
#6  0x46b09b8a in nsDOMDeviceStorage::Init (this=0x518d0f90, aWindow=0x5191ea10, aType=..., aVolName=...) at dom/devicestorage/nsDeviceStorage.cpp:2542
#7  0x46b0a4d0 in nsDOMDeviceStorage::CreateDeviceStorageFor (aWin=0x5191ea10, aType=..., aStore=aStore@entry=0xbeecdacc) at dom/devicestorage/nsDeviceStorage.cpp:2739
#8  0x46765702 in mozilla::dom::Navigator::GetDeviceStorage (this=this@entry=0x518e0240, aType=..., aRv=...) at dom/base/Navigator.cpp:1004
#9  0x4676579e in mozilla::dom::Navigator::GetDeviceStorages (this=this@entry=0x518e0240, aType=..., aStores=..., aRv=...) at dom/base/Navigator.cpp:1028
#10 0x468981c2 in mozilla::dom::NavigatorBinding::getDeviceStorages (cx=0x51050080, obj=..., self=0x518e0240, args=...) at objdir-droid/dom/bindings/NavigatorBinding.cpp:1335
#11 0x46aa953c in mozilla::dom::GenericBindingMethod (cx=0x51050080, argc=<optimized out>, vp=<optimized out>) at dom/bindings/BindingUtils.cpp:2602
#12 0x474a64de in CallJSNative (args=..., native=0x46aa9481 <mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*)>, cx=0x51050080) at js/src/jscntxtinlines.h:235
#13 js::Invoke (cx=0x51050080, args=..., construct=<optimized out>) at js/src/vm/Interpreter.cpp:763
#14 0x474a4186 in Interpret (cx=0x51050080, state=...) at js/src/vm/Interpreter.cpp:3067
#15 0x474a626c in js::RunScript (cx=cx@entry=0x51050080, state=...) at js/src/vm/Interpreter.cpp:704
#16 0x474a634a in js::ExecuteKernel (cx=cx@entry=0x51050080, script=..., scopeChainArg=..., thisv=..., newTargetValue=..., type=type@entry=js::EXECUTE_GLOBAL, evalInFrame=..., result=result@entry=0x0) at js/src/vm/Interpreter.cpp:978
#17 0x474a6414 in js::Execute (cx=cx@entry=0x51050080, script=..., script@entry=..., scopeChainArg=..., rval=rval@entry=0x0) at js/src/vm/Interpreter.cpp:1012
#18 0x4762ad00 in Evaluate (cx=cx@entry=0x51050080, scope=..., staticScope=staticScope@entry=..., optionsArg=..., srcBuf=..., rval=...) at js/src/jsapi.cpp:4447
#19 0x4762addc in Evaluate (cx=cx@entry=0x51050080, scopeChain=..., optionsArg=..., srcBuf=..., rval=...) at js/src/jsapi.cpp:4473
#20 0x4762adf4 in JS::Evaluate (cx=cx@entry=0x51050080, scopeChain=..., optionsArg=..., srcBuf=..., rval=rval@entry=...) at js/src/jsapi.cpp:4529
#21 0x467b7b94 in nsJSUtils::EvaluateString (aCx=0x51050080, aSrcBuf=..., aEvaluationGlobal=..., aEvaluationGlobal@entry=..., aCompileOptions=..., aEvaluateOptions=..., aRetValue=aRetValue@entry=..., aOffThreadToken=aOffThreadToken@entry=0x0) at dom/base/nsJSUtils.cpp:224
#22 0x467b7cb6 in nsJSUtils::EvaluateString (aCx=0x51050080, aSrcBuf=..., aEvaluationGlobal=aEvaluationGlobal@entry=..., aCompileOptions=..., aOffThreadToken=aOffThreadToken@entry=0x0) at dom/base/nsJSUtils.cpp:287
#23 0x467bf83c in nsScriptLoader::EvaluateScript (this=this@entry=0x518cbce0, aRequest=aRequest@entry=0x51e4bbc0, aSrcBuf=...) at dom/base/nsScriptLoader.cpp:1198
#24 0x467bfa24 in nsScriptLoader::ProcessRequest (this=this@entry=0x518cbce0, aRequest=0x51e4bbc0) at dom/base/nsScriptLoader.cpp:1016
#25 0x467c4d60 in nsScriptLoader::ProcessPendingRequests (this=0x518cbce0, this@entry=0x518e8b00) at dom/base/nsScriptLoader.cpp:1255
#26 0x467c4e06 in nsScriptLoader::ParsingComplete (this=0x518e8b00, aTerminated=aTerminated@entry=false) at dom/base/nsScriptLoader.cpp:1667
#27 0x46777212 in nsContentSink::DidBuildModelImpl (this=this@entry=0x518e8b00, aTerminated=<optimized out>) at dom/base/nsContentSink.cpp:1500
#28 0x4661c47a in DidBuildModel (aTerminated=false, this=0x518e8b00) at parser/html/nsHtml5TreeOpExecutor.cpp:137
#29 nsHtml5TreeOpExecutor::DidBuildModel (this=this@entry=0x518e8b00, aTerminated=aTerminated@entry=false) at parser/html/nsHtml5TreeOpExecutor.cpp:113
#30 0x4662ba4a in nsHtml5TreeOperation::Perform (this=this@entry=0x519af9e0, aBuilder=aBuilder@entry=0x518e8b00, aScriptElement=aScriptElement@entry=0xbeece7b4) at parser/html/nsHtml5TreeOperation.cpp:819
#31 0x4662a256 in nsHtml5TreeOpExecutor::RunFlushLoop (this=0x518e8b00) at parser/html/nsHtml5TreeOpExecutor.cpp:447
#32 0x4662a3bc in nsHtml5ExecutorFlusher::Run (this=<optimized out>) at parser/html/nsHtml5StreamParser.cpp:127
#33 0x462d62d2 in ProcessNextEvent (aResult=0xbeece83f, aMayWait=<optimized out>, this=0x4030c380) at xpcom/threads/nsThread.cpp:874
#34 nsThread::ProcessNextEvent (this=0x4030c380, aMayWait=<optimized out>, aResult=0xbeece83f) at xpcom/threads/nsThread.cpp:762
#35 0x462e7b36 in NS_ProcessNextEvent (aThread=<optimized out>, aMayWait=aMayWait@entry=true) at xpcom/glue/nsThreadUtils.cpp:277
#36 0x462d81fa in Shutdown (this=0x518cf7f0) at xpcom/threads/nsThread.cpp:675
#37 nsThread::Shutdown (this=0x518cf7f0) at xpcom/threads/nsThread.cpp:634
#38 0x462b4328 in apply<nsMemoryReporterManager, nsresult (nsMemoryReporterManager::*)()> (m=&virtual nsMemoryReporterManager::UnregisterWeakReporter(nsIMemoryReporter*), o=<optimized out>, this=<optimized out>) at ../../dist/include/nsThreadUtils.h:661
#39 nsRunnableMethodImpl<nsresult (nsMemoryReporterManager::*)(), true>::Run (this=<optimized out>) at ../../dist/include/nsThreadUtils.h:868
#40 0x462d62d2 in ProcessNextEvent (aResult=0xbeece8d7, aMayWait=<optimized out>, this=0x4030c380) at xpcom/threads/nsThread.cpp:874
#41 nsThread::ProcessNextEvent (this=0x4030c380, aMayWait=<optimized out>, aResult=0xbeece8d7) at xpcom/threads/nsThread.cpp:762
#42 0x462e7b36 in NS_ProcessNextEvent (aThread=<optimized out>, aMayWait=aMayWait@entry=true) at xpcom/glue/nsThreadUtils.cpp:277
#43 0x462d81fa in Shutdown (this=0x518cbb90) at xpcom/threads/nsThread.cpp:675
#44 nsThread::Shutdown (this=0x518cbb90) at xpcom/threads/nsThread.cpp:634
#45 0x462b4328 in apply<nsMemoryReporterManager, nsresult (nsMemoryReporterManager::*)()> (m=&virtual nsMemoryReporterManager::UnregisterWeakReporter(nsIMemoryReporter*), o=<optimized out>, this=<optimized out>) at ../../dist/include/nsThreadUtils.h:661
#46 nsRunnableMethodImpl<nsresult (nsMemoryReporterManager::*)(), true>::Run (this=<optimized out>) at ../../dist/include/nsThreadUtils.h:868
#47 0x462d62d2 in ProcessNextEvent (aResult=0xbeece96f, aMayWait=<optimized out>, this=0x4030c380) at xpcom/threads/nsThread.cpp:874
#48 nsThread::ProcessNextEvent (this=0x4030c380, aMayWait=<optimized out>, aResult=0xbeece96f) at xpcom/threads/nsThread.cpp:762
#49 0x462e7b36 in NS_ProcessNextEvent (aThread=<optimized out>, aMayWait=aMayWait@entry=false) at xpcom/glue/nsThreadUtils.cpp:277
#50 0x46424f3c in mozilla::ipc::MessagePump::Run (this=0x403074b0, aDelegate=0xbeecea68) at ipc/glue/MessagePump.cpp:95
#51 0x46416314 in MessageLoop::RunInternal (this=this@entry=0xbeecea68) at ipc/chromium/src/base/message_loop.cc:234
#52 0x464163c8 in RunHandler (this=0xbeecea68) at ipc/chromium/src/base/message_loop.cc:227
#53 MessageLoop::Run (this=0xbeecea68) at ipc/chromium/src/base/message_loop.cc:201
#54 0x46dae4fe in nsBaseAppShell::Run (this=0x4bc82e80) at widget/nsBaseAppShell.cpp:156
#55 0x47135b96 in XRE_RunAppShell () at toolkit/xre/nsEmbedFunctions.cpp:786
#56 0x46416314 in MessageLoop::RunInternal (this=this@entry=0xbeecea68) at ipc/chromium/src/base/message_loop.cc:234
#57 0x464163c8 in RunHandler (this=0xbeecea68) at ipc/chromium/src/base/message_loop.cc:227
#58 MessageLoop::Run (this=this@entry=0xbeecea68) at ipc/chromium/src/base/message_loop.cc:201
#59 0x47135f74 in XRE_InitChildProcess (aArgc=<optimized out>, aArgv=<optimized out>, aGMPLoader=<optimized out>) at toolkit/xre/nsEmbedFunctions.cpp:622
#60 0x40157acc in ChildProcessInit (argc=7, argv=0xbeecf404) at mozglue/android/APKOpen.cpp:427
#61 0x00008440 in main (argc=8, argv=0xbeecf404) at ipc/app/MozillaRuntimeMainAndroid.cpp:34

JS stack:
0 <TOP LEVEL> ["app://ftu.gaiamobile.org/shared/js/contacts/import/utilities/sdcard.js":15]
    this = [object Window]
Attached patch device-storage-oop.patch (obsolete) — Splinter Review
tracking-e10s: - → ---
Attachment #8659653 - Flags: feedback?(cyu)
Comment on attachment 8659653 [details] [diff] [review]
device-storage-oop.patch

FTU starts without crashing with the patch.
Attachment #8659653 - Flags: feedback?(cyu) → feedback+
Attached patch device-storage-oop.patch (obsolete) — Splinter Review
Cleaned up a bit. I tested with this app: https://marketplace.firefox.com/app/file-manager/ and I was able to access files as expected from the sdcard.
Assignee: nobody → fabrice
Attachment #8659653 - Attachment is obsolete: true
Attachment #8660026 - Flags: review?(cyu)
Comment on attachment 8660026 [details] [diff] [review]
device-storage-oop.patch

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

We need another pair of eyes (Reuben?) to check if the change have other issues with the DeviceStorage API.

r- for the following reason, and I'd like to see it addressed.

It's a bad idea to have code excerpts in DeviceStorage.cpp and in ContentChild.cpp doing similar work, one in the chrome process and the other in the content process. It solicits error when there is a need to add new device storage paths. Also, the code added to ContentChild.cpp exposes the inner workings of DeviceStorage, which should be contained in only files implementing the API and not appear in ContentChild.cpp.

Instead of requiring the code in DeviceStorage.cpp to keep in sync with that in ContentChild.cpp, you could: 

1. Extract the init logic in ContentChild.cpp and DeviceStorage.cpp into a method, say DeviceStorage::AndroidInit(), and call it in ContentChild::InitOnContentProcessCreated() and DeviceStorageStatics::InitDirs(). By doing this, even we have to separate initialization blocks for the chrome and content processes, the logic is in the same method and easier to keep in sync.

2. Don't pre-cache the directory names on process startup and load the paths lazily. Implement AndroidBridge::GetExternalPublicDirectory() as:
if (is child process) {
  if (aType is not in ["pictures", "videos", "music", "apps", "sdcard", "crashes"]) return an error.
  if (aType not in the cache) {
    send an IPC message to the chrome process to get the path.
    cache and return the path
  }
}
// here goes the original part for the chrome process.

::: dom/devicestorage/DeviceStorageStatics.cpp
@@ +132,5 @@
>  
>  // Keep MOZ_WIDGET_ANDROID above XP_UNIX,
>  // because both are defined in Android builds.
>  #elif defined (MOZ_WIDGET_ANDROID)
> +  // Keep that in sync with InitOnContentProcessCreated() from ContentChild.cpp

We don't need this comment. Jsut don't repeat yourself by having similar logic in different files.

::: dom/ipc/ContentChild.cpp
@@ +195,5 @@
>  #include "gfxPlatform.h"
>  #include "nscore.h" // for NS_FREE_PERMANENT_DATA
>  
> +#ifdef MOZ_WIDGET_ANDROID
> +# include "AndroidBridge.h"

We don't need to include AndroidBridge.h here because it's the inner workings of DeviceStorage. Usage of AndroidBridge shouldn't appear in this file.

@@ +577,5 @@
> +    AndroidBridge::CacheDeviceStoragePath(NS_LITERAL_STRING(DEVICESTORAGE_MUSIC), path);
> +
> +    child->SendGetDeviceStorageLocation(NS_LITERAL_STRING(DEVICESTORAGE_SDCARD), &path);
> +    AndroidBridge::CacheDeviceStoragePath(NS_LITERAL_STRING(DEVICESTORAGE_SDCARD), path);
> +#endif

Same as above this needs to be extracted into a method called from here or to be eliminated if loaded lazily.
Attachment #8660026 - Flags: review?(cyu) → review-
v2, changing to a lazy getter approach, and removing all changes from ContentChild. AndroidBridge.cpp now directly remotes to the ContentParent. I hope that the sync IPC will not perform too bad...
Attachment #8660026 - Attachment is obsolete: true
Attachment #8660887 - Flags: review?(cyu)
Attachment #8660887 - Flags: feedback?(dhylands)
Comment on attachment 8660887 [details] [diff] [review]
device-storage-oop.patch v2

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

r=me with the following addressed.

::: widget/android/AndroidBridge.cpp
@@ +2132,5 @@
> +            return NS_OK;
> +        }
> +
> +        // Lazily get the value from the parent.
> +         dom::ContentChild* child = dom::ContentChild::GetSingleton();

nit: extra space.

@@ +2136,5 @@
> +         dom::ContentChild* child = dom::ContentChild::GetSingleton();
> +        if (child) {
> +          nsAutoString type(aType);
> +          child->SendGetDeviceStorageLocation(type, &path);
> +          AndroidBridge::sStoragePaths.Put(key, path);

The caching logic is incomplete when you look at this method alone: if the IPC request fails, we'll get an empty value. In this case we shouldn't put the empty value into the cache and fall through to return NS_ERROR_NOT_AVAILABLE below. But this actually works because all possible values of aType are within a limited set, which makes the IPC request always succeed.

Given that, please check whether path is empty or assert that it's always nonempty.
Attachment #8660887 - Flags: review?(cyu) → review+
Comment on attachment 8660887 [details] [diff] [review]
device-storage-oop.patch v2

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

We had a similar problem with the child process getting the volume paths under FxOS.

The way we dealt with it was that when the child starts, we send it some device storage startup information. You can see this in ContentParent::ForwardKnownInfo().

In the past I've used async and sync methods. The sync method added a noticable startup delay (for FxOS), and the async method has races under heavy load. So sending the information when starting the child, and then sending async updates has resolved both of those issues.

Rather than just calling SendVolumes as is being done right now, I think we should have a SendDeviceStoragePathInfo function which includes the Volumes (for gonk) and any additional paths for other environments (IIRC there were also some issues about getting the profile directory from within the child - that may have been resolved elsewhere.
Attachment #8660887 - Flags: feedback?(dhylands) → feedback-
https://hg.mozilla.org/mozilla-central/rev/d6300c4bed63
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Depends on: 1205401
You need to log in before you can comment on or make changes to this bug.