837865 - Device Storage: Use DOMCursor instead of DeviceStorageCursor

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Reuben Morais [:reuben]]

See bug 836519 comment 7.

Comment 1

[Vicamo Yang [:vicamo][:vyang]]

Should SMS use DOMRequestCursor instead once this bug is landed?

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yeah, ideally we should change SMS to use DOMRequest and DOMRequestCursor rather than what it's currently using. That's not terribly important right now though, but something we should make sure to get right in the spec.

Comment 3

[Reuben Morais [:reuben]]

I split this bug in two: instead of refactoring DeviceStorageCursor into a general thing, I implemented DOMCursor in bug 837917 as a smaller, lower risk fix. In this bug we will refactor device storage to use DOMCursor and remove DeviceStorageCursor altogether.

This also means other APIs (bug 838467) can start moving to DOMCursor as soon as it lands.

Comment 4

[Reuben Morais [:reuben]]

Attachment: Use DOMCursor in DeviceStorage, WIP

https://tbpl.mozilla.org/?tree=Try&rev=0c1f6ce452b9

Comment 5

[Reuben Morais [:reuben]]

Comment on attachment 720184 [details] [diff] [review]
Use DOMCursor in DeviceStorage, WIP

>-class PostErrorEvent : public nsRunnable
>+template <typename T>
>+class PostErrorEvent_ : public nsRunnable
…
>+typedef PostErrorEvent_<DOMRequest> PostErrorEvent;
…
>-      nsCOMPtr<PostErrorEvent> event = new PostErrorEvent(mRequest, POST_ERROR_EVENT_FILE_NOT_ENUMERABLE);
>+      nsCOMPtr<PostErrorEvent_<DOMCursor> > event = new PostErrorEvent_<DOMCursor>(mRequest, POST_ERROR_EVENT_FILE_NOT_ENUMERABLE);

Do you think I can avoid this awful hack somehow?

InitCursorEvent creates the PostErrorEvent here: https://hg.mozilla.org/mozilla-central/file/3362afba690e/dom/devicestorage/nsDeviceStorage.cpp#l1052
PostErrorEvent initializes its mRequest (which is an nsRefPtr<DOMRequest>) with aRequest, which asserts inside AddRef: "nsDOMEventTargetHelper is not threadsafe". Everything works fine if mRequest is a nsRefPtr<DOMCursor>.

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

We really need to fix that assertion. Can you grab the stack trace when the assertion fires? Using our environment variable tricks is unreliable, it's best to break here: http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsDebugImpl.cpp#293

Comment 7

[Reuben Morais [:reuben]]

(In reply to ben turner [:bent] from comment #6)
> We really need to fix that assertion. Can you grab the stack trace when the
> assertion fires?

It doesn't call NS_DebugBreak, it uses NS_CheckThreadSafe, which uses MOZ_ASSERT. Anyway, this is the stack:

#0  nsDOMEventTargetHelper::AddRef (this=0x110811568) at /content/events/src/nsDOMEventTargetHelper.cpp:72
#1  0x0000000101d2a0df in mozilla::dom::DOMRequest::AddRef (this=0x110811568) at /dom/base/DOMRequest.cpp:75
#2  0x0000000101d3d01f in nsDOMDeviceStorageCursor::AddRef () at /dom/devicestorage/nsDeviceStorage.cpp:1089
#3  0x0000000101d3d01f in nsRefPtr<nsDOMDeviceStorageCursor>::nsRefPtr () at /dom/devicestorage/nsDeviceStorage.h:904
#4  0x0000000101d3d01f in nsDOMDeviceStorage::EnumerateInternal (this=0x11d4f4f20, aName=>, aOptions=@0x7fff5fbfb748, aCx=>, aArgc=>, aEditable=false) at /dom/devicestorage/nsDeviceStorage.cpp:905
#5  0x0000000101d3d518 in non-virtual thunk to nsDOMDeviceStorage::Enumerate(JS::Value const&, JS::Value const&, JSContext*, unsigned char, nsIDOMDOMCursor**) () at /dom/devicestorage/nsDeviceStorage.cpp:2156
#6  0x0000000102ceddbd in NS_InvokeByIndex_P (that=0x11d4f4f20, methodIndex=>, paramCount=>, params=>) at /xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:162
#7  0x00000001022c0a07 in CallMethodHelper::CallMethodHelper () at /js/xpconnect/src/XPCWrappedNative.cpp:3085
#8  0x00000001022c0a07 in XPCWrappedNative::CallMethod (ccx=>, mode=1606399888) at /js/xpconnect/src/XPCWrappedNative.cpp:2385
#9  0x00000001022c9966 in XPC_WN_CallMethod (cx=>, argc=0, vp=>) at /js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1417
#10 0x0000000103431969 in js::CallJSNative () at /js/src/jscntxtinlines.h:327
#11 0x0000000103431969 in js::InvokeKernel (cx=0x107ab13b0, args={<JS::CallReceiver> = {usedRval_ = false, argv_ = 0x10a0440b8}, argc_ = 0}, construct=>) at jscntxtinlines.h:376
#12 0x000000010342ced2 in js::Interpret (cx=>, entryFrame=>, interpMode=Cannot access memory at address 0x2) at /js/src/jsinterp.cpp:2362
#13 0x0000000103423623 in js::RunScript (cx=>, fp=>) at /js/src/jsinterp.cpp:340
#14 0x0000000103432fb1 in js::ExecuteKernel (cx=0x107ab13b0, scopeChainArg=>, thisv=>, type=>, result=0x0) at /js/src/jsinterp.cpp:530
#15 0x0000000103433292 in js::Execute (cx=0x107ab13b0, scopeChainArg=>, rval=0x0) at /js/src/jsinterp.cpp:569
#16 0x0000000103370cef in JS::Evaluate (cx=0x107ab13b0, options={principals = 0x1133c64b8, originPrincipals = 0x0, version = JSVERSION_DEFAULT, versionSet = true, utf8 = false, filename = 0x11be743c8 "http://mochi.test:8888/tests/dom/devicestorage/test/test_enumerateOptions.html?autorun=1&closeWhenDone=1&logFile=%2FUsers%2FReuben%2FDevelopment%2Fm-c-obj-dbg%2Fmochitest-plain.log&fileLevel=INFO&cons"..., lineno = 24, compileAndGo = true, noScriptRval = true, selfHostingMode = false, userBit = false, sourcePolicy = JS::CompileOptions::SAVE_SOURCE}, chars=0x11c310208, length=>, rval=0x0) at /js/src/jsapi.cpp:5557
#17 0x0000000101c73bc3 in nsJSContext::EvaluateString (this=0x107ab00c0, aScript=@0x7fff5fbfcf18, aScopeObject=@0x107ab13b0, aOptions=>, aCoerceToString=false, aRetValue=0x0) at /dom/base/nsJSEnvironment.cpp:1289
#18 0x00000001018a289b in nsScriptLoader::EvaluateScript (this=>, aRequest=>, aScript=@0x7fff5fbfcf18) at /content/base/src/nsScriptLoader.cpp:847
#19 0x00000001018a1f59 in nsScriptLoader::ProcessRequest (this=0x1133ca510, aRequest=0x11bed1840) at /content/base/src/nsScriptLoader.cpp:737
#20 0x00000001018a2d49 in nsCOMPtr<nsIScriptElement>::operator-> () at -obj-dbg/dist/include/nsCOMPtr.h:879
#21 0x00000001018a2d49 in nsScriptLoader::ContinueParserAsync () at /content/base/src/nsScriptLoader.h:1119
#22 0x00000001018a2d49 in nsScriptLoader::ProcessPendingRequests (this=0x1133ca510) at /content/base/src/nsScriptLoader.cpp:880
#23 0x00000001018a4d36 in nsRunnableMethodImpl<void (nsScriptLoader::*)(), true>::Run (this=>) at nsThreadUtils.h:367
#24 0x0000000102ccdd0e in nsThread::ProcessNextEvent (this=0x1001247b0, mayWait=>, result=0x7fff5fbfd107) at /xpcom/threads/nsThread.cpp:627
#25 0x0000000102c6e918 in NS_ProcessPendingEvents_P (thread=>, timeout=20) at nsThreadUtils.cpp:188
#26 0x00000001026f3787 in nsBaseAppShell::NativeEventCallback (this=0x100150420) at /widget/xpwidgets/nsBaseAppShell.cpp:97
#27 0x00000001026a907f in nsAppShell::ProcessGeckoEvents (aInfo=0x100150420) at /widget/cocoa/nsAppShell.mm:387

Comment 8

[Reuben Morais [:reuben]]

Attachment: Use DOMCursor in DeviceStorage, v0

I fixed it by changing less things ;)

Comment 9

[Reuben Morais [:reuben]]

Attachment: Use DOMCursor in DeviceStorage

Shameless bump disguised as a rebase to tip.

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Reuben Morais [:reuben] from comment #9)
> Shameless bump disguised as a rebase to tip.

FYI that moved this request to the end of my queue :)

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 723005 [details] [diff] [review]
Use DOMCursor in DeviceStorage

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

One thing I noticed is that nsIDOMDOMCursor doesn't inherit nsIDOMDOMRequest. Is there a reason for that? I think it probably should.

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +1028,5 @@
> +  if (file) {
> +    cursor->mOkToCallContinue = true;
> +    mRequest->FireSuccess(val);
> +  } else {
> +    static_cast<nsDOMDeviceStorageCursor*>(mRequest.get())->FireDone();

Just use 'cursor' here instead of casting again.

@@ +2220,5 @@
>    nsRefPtr<nsDOMDeviceStorageCursor> cursor = new nsDOMDeviceStorageCursor(win, mPrincipal,
>                                                                             dsf, since);
>    nsRefPtr<DeviceStorageCursorRequest> r = new DeviceStorageCursorRequest(cursor);
>  
> +  NS_ADDREF(*aRetval = static_cast<nsIDOMDOMCursor*>(cursor));

Is this cast necessary?

::: dom/devicestorage/nsDeviceStorage.h
@@ +97,5 @@
>    NS_DECL_ISUPPORTS_INHERITED
>    NS_DECL_NSICONTENTPERMISSIONREQUEST
> +
> +  // nsIDOMDOMCursor methods
> +  NS_IMETHOD GetDone(bool* aDone) { return DOMCursor::GetDone(aDone); }

You don't need to override this, it'll call the DOMCursor impl automatically. Maybe just comment that DOMCursor::GetDone is sufficient.

@@ +98,5 @@
>    NS_DECL_NSICONTENTPERMISSIONREQUEST
> +
> +  // nsIDOMDOMCursor methods
> +  NS_IMETHOD GetDone(bool* aDone) { return DOMCursor::GetDone(aDone); }
> +  NS_IMETHOD Continue(void);

Nit: Add MOZ_OVERRIDE here so that you know your version matches the virtual.

Comment 12

[Reuben Morais [:reuben]]

(In reply to ben turner [:bent] from comment #11)
> One thing I noticed is that nsIDOMDOMCursor doesn't inherit
> nsIDOMDOMRequest. Is there a reason for that? I think it probably should.

Jonas advised me not to do that because there was some XPCOM craziness involved when using it from C++ if I did that. I'm only using the API from JS, so I don't know what the craziness is. Jonas?

Comment 13

[Reuben Morais [:reuben]]

Attachment: Use DOMCursor in DeviceStorage

Thanks for the review!

Comment 14

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 724591 [details] [diff] [review]
Use DOMCursor in DeviceStorage

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

r=me, thanks!

::: dom/devicestorage/nsDeviceStorage.h
@@ +98,5 @@
>    NS_DECL_NSICONTENTPERMISSIONREQUEST
> +
> +  // nsIDOMDOMCursor interface.
> +  // We use DOMCursor::GetDone.
> +  NS_IMETHOD Continue(void) MOZ_OVERRIDE;

Nit: IMO the 'void' here is just clutter. I'd remove it, but I don't mind if you want to leave it.

Comment 15

[Reuben Morais [:reuben]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a66d427eb0a1

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a66d427eb0a1