Closed Bug 1113062 Opened 7 years ago Closed 7 years ago

crash in mozilla::dom::FileHandleBase::IsOpen()

Categories

(Core :: DOM: Core & HTML, defect)

33 Branch
x86
Windows NT
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
firefox34 --- wontfix
firefox35 + wontfix
firefox36 + fixed
firefox37 + fixed

People

(Reporter: epinal99-bugzilla2, Assigned: baku)

References

Details

(Keywords: crash, regression, testcase)

Crash Data

Attachments

(3 files, 7 obsolete files)

This bug was filed from the Socorro interface and is 
report bp-0f532c04-2f51-4e1a-9672-180fd2141218.
=============================================================


Reported here https://bugzilla.mozilla.org/show_bug.cgi?id=898856#c8

STR: open https://bugzilla.mozilla.org/attachment.cgi?id=8538260 and click the button (a few times if you want, you can open the web console)

Result: crash in [@ mozilla::dom::FileHandleBase::IsOpen() ]

Regression range:
good=2014-06-27
bad=2014-06-28
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=be076357691c&tochange=2f5df65e3662

My guess goes to:
Andrea Marchesini — Bug 1030844 - Get rid of nsIDOMWindowUtils.getFile/getBlob, r=janv
Blocks: 1030844
Flags: needinfo?(amarchesini)
Keywords: regression, testcase
bent/janv, it seems a regression in IDB.
Flags: needinfo?(amarchesini) → needinfo?(Jan.Varga)
It turned out that we call:
mozilla::dom::indexedDB::FileImplSnapshot::GetInternalStream

after this:
mozilla::dom::indexedDB::FileImplSnapshot::Unlink

and at that point mFileHandle is null.
Calling CC/GC from about:memory can easily reproduce this issue.
We're too late for 35 at this point, Andrea can we get an assignee on this issue now that we know how to reproduce so that an uplift is possible for 36?
Flags: needinfo?(amarchesini)
I'll discuss about this bug with janv e bent. For today we will have an assignee.
Flags: needinfo?(amarchesini)
Attached patch crash.patch (obsolete) — Splinter Review
actually this issue exists since the porting of DOMFile/DOMBlob to webidl, I guess.

The issue is that we have 2 FileImpl implementations that need to be unlinked/traversed: ArchiveReader and File in IDB. Both keep a pointer of some CCed obj.

With this patch we revoke the blob URL of something generated by this kind of File. This is an easy fix but probably it's not what we want on a long run.
Attachment #8543941 - Flags: review?(bugs)
Comment on attachment 8543941 [details] [diff] [review]
crash.patch


>+  if (tmp->mImpl->IsCCed()) {
>+    tmp->mImpl->Unlink();
>+    nsHostObjectProtocolHandler::InvalidateObject(tmp->mImpl);
But can't we have references to tmp->mImpl elsewhere.
As discussed on IRC, we want the other approach.


>-static nsISupports*
>+namespace {
Why this? Per coding style we're supposed to avoid anonymous namespaces.
Attachment #8543941 - Flags: review?(bugs) → review-
Attached patch crash.patch (obsolete) — Splinter Review
I need to use an integer because I can generate more than 1 URL from the same blob.
Attachment #8543941 - Attachment is obsolete: true
Attachment #8543965 - Flags: review?(bugs)
Flags: needinfo?(Jan.Varga)
Attached patch crash.patch (obsolete) — Splinter Review
Attachment #8543965 - Attachment is obsolete: true
Attachment #8543965 - Flags: review?(bugs)
Attachment #8544104 - Flags: review?(bugs)
Comment on attachment 8544104 [details] [diff] [review]
crash.patch

>+  bool HasZeroRefCount() const
>+  {
>+    return !!mObjectRefCount;
>+  }
This is odd name, since the it has nothing to do with the refcnt of the object on which it is called.
Maybe HasOwners()

> URL::CreateObjectURL(const GlobalObject& aGlobal, File& aBlob,
>                      const mozilla::dom::objectURLOptions& aOptions,
>                      nsString& aResult, mozilla::ErrorResult& aRv)
> {
>+  if (aBlob.Impl()->IsCCed()) {
>+    // The creation of a blob URL happens on the main-thread and we cannot
>+    // transfer a FileImpl if CCed.
>+    aRv.Throw(NS_ERROR_FAILURE);
>+    return;
>+  }
Please file a followup bug for this, since we shouldn't have weird special cases for certain types of files.
Attachment #8544104 - Flags: review?(bugs) → review+
Attachment #8544104 - Flags: review+ → review?(bugs)
Comment on attachment 8544104 [details] [diff] [review]
crash.patch

> NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(File)
>   MOZ_ASSERT(tmp->mImpl);
>-  tmp->mImpl->Unlink();
>+  if (tmp->mImpl->IsCCed()) {
>+    tmp->mImpl->ReleaseObject();
>+    if (!tmp->mImpl->HasZeroRefCount()) {
>+      tmp->mImpl->Unlink();
>+    }
>+  }
>   NS_IMPL_CYCLE_COLLECTION_UNLINK(mParent)
>   NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER
> NS_IMPL_CYCLE_COLLECTION_UNLINK_END
> 
> NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(File)
>   MOZ_ASSERT(tmp->mImpl);
>-  tmp->mImpl->Traverse(cb);
>+  if (tmp->mImpl->IsCCed()) {
>+    tmp->mImpl->Traverse(cb);
>+  }
Wait, I still don't understand this.
We may end up calling Impl::Traverse on several File objects and whatever Impl keeps alive would get too many
internal references, since tmp->mImpl->Traverse effectively tells to cycle collector that tmp owns the strong member variables
of mImpl (but mImpl has addrefed those objects only once, they aren't not addrefed as many times as there are File objects).


http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsCycleCollector.cpp?rev=9997dbfbc68a&mark=3149-3149#3115
Attachment #8544104 - Flags: review?(bugs) → review-
Assignee: nobody → amarchesini
I prefer to keep this separate. a second patch is coming.
Attachment #8545873 - Flags: review?(bugs)
Attached patch patch 2 - CC/MT FileImplBase (obsolete) — Splinter Review
This doesn't cover MultipartFileImpl and RemoteBlob
Attachment #8544104 - Attachment is obsolete: true
Attachment #8545932 - Flags: review?(bugs)
Comment on attachment 8545873 [details] [diff] [review]
patch 1 - PIFileImpl merged with FileImpl

>-#define PIFILEIMPL_IID \
>+#define FILEIMPL_IID \
>   { 0x218ee173, 0xf44f, 0x4d30, \
>     { 0xab, 0x0c, 0xd6, 0x66, 0xea, 0xc2, 0x84, 0x47 } }
please update IID, just in case some binary addon happens to use it.


>+++ b/dom/base/nsHostObjectProtocolHandler.cpp
>@@ -494,17 +494,17 @@ nsHostObjectProtocolHandler::NewChannel2
>   uri->GetSpec(spec);
> 
>   DataInfo* info = GetDataInfo(spec);
> 
>   if (!info) {
>     return NS_ERROR_DOM_BAD_URI;
>   }
> 
>-  nsCOMPtr<PIFileImpl> blobImpl = do_QueryInterface(info->mObject);
>+  nsCOMPtr<FileImpl> blobImpl = do_QueryInterface(info->mObject);
s/blobImpl/blob/
and then you can remove 
FileImpl* blob = static_cast<FileImpl*>(blobImpl.get());
later in the method.


> NS_GetBlobForBlobURI(nsIURI* aURI, FileImpl** aBlob)
> {
>   NS_ASSERTION(IsBlobURI(aURI), "Only call this with blob URIs");
> 
>   *aBlob = nullptr;
> 
>-  nsCOMPtr<PIFileImpl> blobImpl = do_QueryInterface(GetDataObject(aURI));
>+  nsCOMPtr<FileImpl> blobImpl = do_QueryInterface(GetDataObject(aURI));
s/blobImpl/blob/

>   if (!blobImpl) {
>     return NS_ERROR_DOM_BAD_URI;
>   }
> 
>   nsRefPtr<FileImpl> blob = static_cast<FileImpl*>(blobImpl.get());
and then you can remove this part
Attachment #8545873 - Flags: review?(bugs) → review+
Comment on attachment 8545932 [details] [diff] [review]
patch 2 - CC/MT FileImplBase

> NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(File)
>   MOZ_ASSERT(tmp->mImpl);
>-  tmp->mImpl->Unlink();
>+
>+  if (tmp->mImpl->IsCCed()) {
>+    NS_IMPL_CYCLE_COLLECTION_UNLINK(mImpl)
>+  }
You shouldn't need if (tmp->mImpl->IsCCed()) {.
We certainly want to unlink (i.e. set mImpl null here) always

> NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(File)
>   MOZ_ASSERT(tmp->mImpl);
>-  tmp->mImpl->Traverse(cb);
>+
>+  if (tmp->mImpl->IsCCed()) {
>+    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mImpl)
>+  }
And here traversing such mImpl which isn't cycle collectable just doesn't do anything.
So you could just drop if (tmp->mImpl->IsCCed()) {


>-NS_IMPL_ISUPPORTS_INHERITED0(FileImplMemory, FileImpl)
>+NS_IMPL_ISUPPORTS_INHERITED0(FileImplMemory, FileImplBaseMT)
Oh, we used to have a small bug here (which doesn't affect to anything in reality).

>@@ -2023,23 +2023,23 @@ class BlobParent::RemoteBlobImpl MOZ_FIN
>   : public FileImpl
>   , public nsIRemoteBlob
> {
>   BlobParent* mActor;
>   nsCOMPtr<nsIEventTarget> mActorTarget;
>   nsRefPtr<FileImpl> mBlobImpl;
> 
> public:
>+  NS_DECL_THREADSAFE_ISUPPORTS
You shouldn't have this.
FileImplBaseMT already declares and defines the threadsafe refcnting.


> NS_IMPL_ADDREF(BlobChild::RemoteBlobImpl)
> NS_IMPL_RELEASE_WITH_DESTROY(BlobChild::RemoteBlobImpl, Destroy())
Ahaa, you want Destroy().
(1) One option is to use NS_IMPL_RELEASE_WITH_DESTROY in FileImplBaseMT
and then add virtual void Destroy() {} to FileImplBaseMT and override it in
BlobChild::RemoteBlobImpl.
(2) Other option, which I think should work too, is to use 
NS_DECL_ISUPPORTS_INHERITED in class BlobChild::RemoteBlobImpl and then
leave NS_IMPL_ADDREF and NS_IMPL_RELEASE_WITH_DESTROY as they are in the patch


> NS_IMPL_ADDREF(BlobParent::RemoteBlobImpl)
> NS_IMPL_RELEASE_WITH_DESTROY(BlobParent::RemoteBlobImpl, Destroy())
Same here



....oh, only BlobParent thing has NS_DECL_THREADSAFE_ISUPPORTS in the patch.
BlobChild part looks ok, so do the same thing in BlobParent stuff too.
So it would be (2)


Those fixed, r=me
Attachment #8545932 - Flags: review?(bugs) → review+
Attached patch patch 3 - multipartFileImpl (obsolete) — Splinter Review
same approach as FileImplBaseMT/CC
Attachment #8545978 - Flags: review?(bugs)
Comment on attachment 8545932 [details] [diff] [review]
patch 2 - CC/MT FileImplBase

Ok, BlobParent::RemoteBlobImpl doesn't inherit *MT so it needs NS_DECL_THREADSAFE_ISUPPORTS. My mistake.
Comment on attachment 8545978 [details] [diff] [review]
patch 3 - multipartFileImpl

>+static bool
>+SequenceIsCCed(const Sequence<OwningArrayBufferOrArrayBufferViewOrBlobOrString>& aData)
Perhaps call this SequenceContainsCCedFileImpls


>+{
>+  for (uint32_t i = 0, len = aData.Length(); i < len; ++i) {
>+    const OwningArrayBufferOrArrayBufferViewOrBlobOrString& data = aData[i];
>+
>+    if (data.IsBlob()) {
>+      nsRefPtr<File> file = data.GetAsBlob().get();
I don't recall now whether get() returns already_AddRefed.
If not, use File* file.

>+bool
>+MultipartFileImpl::IsCCed() const
>+{
>+  for (uint32_t i = 0; i < mBlobImpls.Length(); ++i) {
>+    if (mBlobImpls[i]->IsCCed()) {
>+      return true;
>+    }
>+  }
>+
>+  return false;
>+}
leftover code. remove
Attachment #8545978 - Flags: review?(bugs) → review+
I fully support patch 1, but I am vehemently opposed to making FileImpl anything other than 100% threadsafe. CC'd stuff belongs on the File objects, not the FileImple objects!
Talking with bent on IRC, we decided that it's better to remove IsCCed() from FileImpl completely. At the moment  we have ArchiveReader and FileSnapshot.

Here the patch to make ArchiveReaderZipFile non-CCed.
Attachment #8546179 - Flags: review?(bugs)
janv, how hard is to make FileSnapshotImpl non-CCed? Removing any reference to IDBFileHandle?
Flags: needinfo?(Jan.Varga)
mFileHandle is quite important member, since the whole point of FileImplSnapshot is to check mFileHandle->IsOpen() in GetInternalStream()
Flags: needinfo?(Jan.Varga)
(In reply to Jan Varga [:janv] from comment #23)
> mFileHandle is quite important member, since the whole point of
> FileImplSnapshot is to check mFileHandle->IsOpen() in GetInternalStream()

We're going to have to find another way to implement FileImplSnapshot without that knowledge, then.
Or maybe just forbid FileImplSnapshot from being cloned to another thread?
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #25)
> Or maybe just forbid FileImplSnapshot from being cloned to another thread?

IsCCed() was added later to prevent passing such blobs to workers, but I recall that the original problem was a cycle created by the FileImplSnapshot, it was holding entire nsGlobalWindow.
Maybe the cycle is not created anymore, try to remove CC stuff from FileSnapshotImpl and test locally and then on try.
Does FileSnapshotImpl need to keep IDBFileHandle alive, or would it be possible 
to split IDBFileHandle to IDBFileHandle and FileHandleBase?
IDBFileHandle would have a strong reference to FileHandleBase.
(In reply to Olli Pettay [:smaug] from comment #27)
> Does FileSnapshotImpl need to keep IDBFileHandle alive, or would it be
> possible 
> to split IDBFileHandle to IDBFileHandle and FileHandleBase?
> IDBFileHandle would have a strong reference to FileHandleBase.

As I said on IRC, I think that FileImplSnapshot doesn't need a strong ref to IDBFileHandle.
Attachment #8546179 - Flags: review?(bugs) → review+
Attachment #8545932 - Attachment is obsolete: true
Attachment #8545978 - Attachment is obsolete: true
Attachment #8546236 - Flags: review?(Jan.Varga)
Comment on attachment 8546236 [details] [diff] [review]
patch 3 - FileSnapshotImpl no CCed

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

I went quickly through the patch, the approach looks good to me. I'll have a closer look tomorrow.
Can you push to try in the meantime ?
Comment on attachment 8546236 [details] [diff] [review]
patch 3 - FileSnapshotImpl no CCed

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

::: dom/indexedDB/FileSnapshot.cpp
@@ +51,5 @@
>  
>    mFileInfos.AppendElement(aFileInfo);
> +
> +  nsCOMPtr<EventTarget> et = aFileHandle;
> +  mFileHandle = do_GetWeakReference(et);

mFileHandle is null here, IDBFileHandle must inherit nsSupportsWeakReference (directly or indirectly) I think
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=08c99b19841f
Attachment #8546236 - Attachment is obsolete: true
Attachment #8546236 - Flags: review?(Jan.Varga)
Attachment #8547028 - Flags: review?(Jan.Varga)
Comment on attachment 8547028 [details] [diff] [review]
patch 3 - FileSnapshotImpl no CCed

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

::: dom/base/File.h
@@ +286,5 @@
>  
>    virtual bool IsFile() const = 0;
>  
> +  // True if this implementation can be sent to other threads.
> +  virtual bool IsTransferrable() const

My only real comment here is that "transferable" is a term that already has meaning with respect to structured clones. "Transferable" means that you can neuter+clone, so I think we should come up with a better name for this. "MayBeClonedToOtherThreads"? Something along those lines.

Also, "transferrable" is misspelled, it's "transferable". English is awesome ;)
Comment on attachment 8547028 [details] [diff] [review]
patch 3 - FileSnapshotImpl no CCed

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

Looks good, but there are some unlink related crashes on windows debug, plus what bent said.

::: dom/base/File.cpp
@@ +129,5 @@
>  NS_IMPL_CYCLE_COLLECTION_CLASS(File)
>  
>  NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(File)
>    MOZ_ASSERT(tmp->mImpl);
> +  NS_IMPL_CYCLE_COLLECTION_UNLINK(mImpl)

Unlink, but no Traverse ?

::: dom/indexedDB/FileSnapshot.cpp
@@ +51,5 @@
>  
>    mFileInfos.AppendElement(aFileInfo);
> +
> +  nsCOMPtr<EventTarget> et = aFileHandle;
> +  mFileHandle = do_GetWeakReference(et);

What about:
mFileHandle =
  do_GetWeakReference(NS_ISUPPORTS_CAST(EventTarget*, aFileHandle));

@@ +79,5 @@
>  
>    mFileInfos.AppendElement(fileInfo);
> +
> +  nsCOMPtr<EventTarget> et = aFileHandle;
> +  mFileHandle = do_GetWeakReference(et);

dtto

@@ +107,5 @@
>    AssertSanity();
>  
> +  nsRefPtr<IDBFileHandle> fileHandle = GetIDBFileHandle(mFileHandle);
> +  if (!fileHandle) {
> +    return NS_ERROR_FAILURE;

I think we should return NS_ERROR_DOM_FILEHANDLE_INACTIVE_ERR here.

@@ +131,5 @@
> +  nsRefPtr<IDBFileHandle> fileHandle = GetIDBFileHandle(mFileHandle);
> +  if (!fileHandle) {
> +    aRv.Throw(NS_ERROR_FAILURE);
> +    return nullptr;
> +  }

Hmm, CreateSlice() doesn't need to access content of the blob.
Can't you just pass the weak ref to the sliced blob ?
If you do that, then you can remove GetIDBFileHandle() helper and do the query referent in GetInternalStream().
Attachment #8547028 - Flags: review?(Jan.Varga)
Comment on attachment 8547151 [details] [diff] [review]
patch 3 - FileSnapshotImpl no CCed

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

Hopefully, it passes on try.

::: dom/base/File.h
@@ +286,5 @@
>  
>    virtual bool IsFile() const = 0;
>  
> +  // True if this implementation can be sent to other threads.
> +  virtual bool IsMayBeClonedToOtherThreads() const

Without *Is* sounds better to me.

::: dom/indexedDB/FileSnapshot.cpp
@@ +43,5 @@
>  
>    mFileInfos.AppendElement(aFileInfo);
> +
> +  mFileHandle =
> +  do_GetWeakReference(NS_ISUPPORTS_CAST(EventTarget*, aFileHandle));

missing indentation (2 spaces)
Attachment #8547151 - Flags: review?(Jan.Varga) → review+
Comment on attachment 8547151 [details] [diff] [review]
patch 3 - FileSnapshotImpl no CCed

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

::: dom/indexedDB/FileSnapshot.cpp
@@ +95,5 @@
>  {
>    AssertSanity();
>  
> +  nsCOMPtr<EventTarget> et = do_QueryReferent(mFileHandle);
> +  nsRefPtr<IDBFileHandle> fileHandle = static_cast<IDBFileHandle*>(et.get());

Hm, this can be now a raw pointer I think:
IDBFileHandle* fileHandle = static_cast<IDBFileHandle*>(et.get());
Andrea, could you fill the uplift request? We would like to see that fixed for 36. Thanks
Flags: needinfo?(amarchesini)
Comment on attachment 8545873 [details] [diff] [review]
patch 1 - PIFileImpl merged with FileImpl

Approval Request Comment
[Feature/regressing bug #]: bug 1047483
[User impact if declined]: a leak and an broken blobs.
[Describe test coverage new/current, TBPL]: We made FileImpl fully non-CCed. tbpl tests are all green.
[Risks and why]: The patch is easy, I don't see risks.
[String/UUID change made/needed]: none
Flags: needinfo?(amarchesini)
Attachment #8545873 - Flags: approval-mozilla-aurora?
Attachment #8546179 - Flags: approval-mozilla-aurora?
Comment on attachment 8547151 [details] [diff] [review]
patch 3 - FileSnapshotImpl no CCed

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
[Describe test coverage new/current, TBPL]:
[Risks and why]: 
[String/UUID change made/needed]:
Attachment #8547151 - Flags: approval-mozilla-aurora?
Aurora is OK, or do we want beta too?
Flags: needinfo?(sledru)
yes, we want beta (36) too, thanks
Flags: needinfo?(sledru)
Comment on attachment 8545873 [details] [diff] [review]
patch 1 - PIFileImpl merged with FileImpl

Approval Request Comment
[Feature/regressing bug #]: bug 1047483
[User impact if declined]: a leak and an broken blobs.
[Describe test coverage new/current, TBPL]: We made FileImpl fully non-CCed. tbpl tests are all green.
[Risks and why]: The patch is easy, I don't see risks.
[String/UUID change made/needed]: none
Attachment #8545873 - Flags: approval-mozilla-beta?
Attachment #8546179 - Flags: approval-mozilla-beta?
Attachment #8547151 - Flags: approval-mozilla-beta?
Comment on attachment 8545873 [details] [diff] [review]
patch 1 - PIFileImpl merged with FileImpl

We are after the merge, aurora is 37, beta is 36.
Attachment #8545873 - Flags: approval-mozilla-beta?
Attachment #8545873 - Flags: approval-mozilla-beta+
Attachment #8545873 - Flags: approval-mozilla-aurora?
Attachment #8545873 - Flags: approval-mozilla-aurora-
Attachment #8546179 - Flags: approval-mozilla-beta?
Attachment #8546179 - Flags: approval-mozilla-beta+
Attachment #8546179 - Flags: approval-mozilla-aurora?
Attachment #8546179 - Flags: approval-mozilla-aurora-
Attachment #8547151 - Flags: approval-mozilla-beta?
Attachment #8547151 - Flags: approval-mozilla-beta+
Attachment #8547151 - Flags: approval-mozilla-aurora?
Attachment #8547151 - Flags: approval-mozilla-aurora-
Duplicate of this bug: 1083828
Duplicate of this bug: 1083349
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.