1204107 - IDB should use StructuredCloneHelperInternal

Status: NEW

(Core :: Storage: IndexedDB, task, P3)

Comment 1

[Andrea Marchesini [:baku]]

Attachment: idb.patch

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8660264 [details] [diff] [review]
idb.patch

+  // This is for special cases like IDB.
+  bool ReadFromBuffer(JSContext* aCx,
+                      uint64_t* aBuffer,
+                      size_t aBufferLength,
+                      JS::MutableHandle<JS::Value> aValue);
+
+  bool ReadFromBuffer(JSContext* aCx,
+                      uint64_t* aBuffer,
+                      size_t aBufferLength,
+                      uint32_t aAlgorithmVersion,
+                      JS::MutableHandle<JS::Value> aValue);
+
So both those methods are for IDB or only the first one?
And what do the methods do differently comparing to other Read*?
We have two ReadFromBuffer methods also in StructuredCloneHelper. How are these different?
This setup is rather unclear. The methods in StructuredCloneHelper are used by IPC code, but the ones in 
StructuredCloneHelperInternal are used by IDB.



>+    // Try using the runtime callbacks
>+    const JSStructuredCloneCallbacks* runtimeCallbacks =
>+      js::GetContextStructuredCloneCallbacks(aCx);
>+    if (runtimeCallbacks) {
>+      return runtimeCallbacks->write(aCx, aWriter, aObj, nullptr);
>+    }
Aha, this code makes sense only before Bug 1203916 lands.


>+StructuredCloneReadInfo::ReadCallback(JSContext* aCx,
>+                                      JSStructuredCloneReader* aReader,
>+                                      uint32_t aTag,
>+                                      uint32_t aIndex)
>+{
>+  // We need to statically assert that our tag values are what we expect
>+  // so that if people accidentally change them they notice.
>+  static_assert(SCTAG_DOM_BLOB == 0xffff8001 &&
>+                SCTAG_DOM_FILE_WITHOUT_LASTMODIFIEDDATE == 0xffff8002 &&
>+                SCTAG_DOM_MUTABLEFILE == 0xffff8004 &&
>+                SCTAG_DOM_FILE == 0xffff8005,
>+                "You changed our structured clone tag values and just ate "
>+                "everyone's IndexedDB data.  I hope you are happy.");
>+
>+  if (aTag == SCTAG_DOM_FILE_WITHOUT_LASTMODIFIEDDATE ||
>+      aTag == SCTAG_DOM_BLOB ||
>+      aTag == SCTAG_DOM_FILE ||
>+      aTag == SCTAG_DOM_MUTABLEFILE) {
>+    if (aIndex >= mFiles.Length()) {
>+      MOZ_ASSERT(false, "Bad index value!");
>+      return nullptr;
>+    }
>+
>+    StructuredCloneFile& file = mFiles[aIndex];
>+
>+    JS::Rooted<JSObject*> result(aCx);
>+
>+    if (aTag == SCTAG_DOM_MUTABLEFILE) {
>+      MutableFileData data;
>+      if (NS_WARN_IF(!ReadFileHandle(aReader, &data))) {
>+        return nullptr;
>+      }
>+
>+      bool done = false;
>+      switch (mDeserializationType) {
>+        case ValueDeserialization:
>+          done = ValueDeserializationHelper::CreateAndWrapMutableFile(aCx,
>+                                                                      file,
>+                                                                      data,
>+                                                                      &result);
>+          break;
>+
>+        case IndexDeserialization:
>+          done = IndexDeserializationHelper::CreateAndWrapMutableFile(aCx,
>+                                                                      file,
>+                                                                      data,
>+                                                                      &result);
>+          break;
>+
>+#if !defined(MOZ_B2G)
>+        case UpgradeDeserialization:
>+          done = UpgradeDeserializationHelper::CreateAndWrapMutableFile(aCx,
>+                                                                        file,
>+                                                                        data,
>+                                                                        &result);
>+          break;
>+#endif
>+        default:
>+          MOZ_CRASH("What?");
>+          break;
>+      }
>+
>+      if (NS_WARN_IF(!done)) {
>+        return nullptr;
>+      }
>+
>+      return result;
>+    }
>+
>+    BlobOrFileData data;
>+    if (NS_WARN_IF(!ReadBlobOrFile(aReader, aTag, &data))) {
>+      return nullptr;
>+    }
>+
>+    bool done = false;
>+    switch (mDeserializationType) {
>+      case ValueDeserialization:
>+        done = ValueDeserializationHelper::CreateAndWrapBlobOrFile(aCx,
>+                                                                   mDatabase,
>+                                                                   file,
>+                                                                   data,
>+                                                                   &result);
>+        break;
>+
>+      case IndexDeserialization:
>+        done = IndexDeserializationHelper::CreateAndWrapBlobOrFile(aCx,
>+                                                                   mDatabase,
>+                                                                   file,
>+                                                                   data,
>+                                                                   &result);
>+        break;
>+
>+#if !defined(MOZ_B2G)
>+      case UpgradeDeserialization:
>+        done = UpgradeDeserializationHelper::CreateAndWrapBlobOrFile(aCx,
>+                                                                     mDatabase,
>+                                                                     file,
>+                                                                     data,
>+                                                                     &result);
>+        break;
>+#endif
>+      default:
>+        MOZ_CRASH("What?");
>+        break;
>+    }
Why is this code so much more repetitive in the new method comparing to the old one?

Comment 3

[Andrea Marchesini [:baku]]

> So both those methods are for IDB or only the first one?
> And what do the methods do differently comparing to other Read*?
> We have two ReadFromBuffer methods also in StructuredCloneHelper. How are
> these different?

StructuredCloneHelper::ReadFromBuffer uses these 2 methods.
The difference is that CloneHelper has to set the parent and it returns and ErrorResult after cleaning the exceptions.

StructuredCloneHelperInternal::ReadFromBuffer is just a small layer on top of JS_ReadStructuredClone.

I think we need both.

> This setup is rather unclear. The methods in StructuredCloneHelper are used
> by IPC code, but the ones in 
> StructuredCloneHelperInternal are used by IDB.

Yes, IDB uses the same IPC pattern. But it's so custom that I didn't find any way to merge those pieces.

> Aha, this code makes sense only before Bug 1203916 lands.

Ah yeah. sorry. 

> Why is this code so much more repetitive in the new method comparing to the
> old one?

Because the previous code was based on templates. We cannot have templates with the new approach. I used an enum instead.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

So could we try to rename ReadFromBuffer methods at least. I mean, it is very hard to understand what the protected ReadFromBuffer methods are for vs what the public methods in StructuredCloneHelperInternal are for.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: idb.patch

I made those 2 methods protected, and renamed to ReadFromExternalBuffer.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

And how is ReadFromExternalBuffer different from ReadFromBuffer? Both read from a buffer.

+  bool ReadFromExternalBuffer(JSContext* aCx,
+                              uint64_t* aBuffer,
+                              size_t aBufferLength,
+                              uint32_t aAlgorithmVersion,
+                              JS::MutableHandle<JS::Value> aValue);
vs
  void ReadFromBuffer(nsISupports* aParent,
                      JSContext* aCx,
                      uint64_t* aBuffer,
                      size_t aBufferLength,
                      uint32_t aAlgorithmVersion,
                      JS::MutableHandle<JS::Value> aValue,
                      ErrorResult &aRv);

Do we really need both ReadFromExternalBuffer and ReadFromBuffer ?

Sorry, I think I'm missing something here. What is causing this complexity/code duplication?
Why can't StructuredCloneReadInfo::Read use the existing methods? Does StructuredCloneReadInfo have to inherit StructuredCloneHelperInternal and not StructuredCloneHelper?

Comment 7

[Andrea Marchesini [:baku]]

Let's talk on IRC about this.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8660429 [details] [diff] [review]
idb.patch

We're postponing this change because of IDB code being too weird.

Comment 9

[Jens Stutte [:jstutte]]

(In reply to Olli Pettay [:smaug] from comment #8)

Comment on attachment 8660429 [details] [diff] [review]
idb.patch

We're postponing this change because of IDB code being too weird.

I assume IDB code changed a lot since then. Is this still a thing, Olli?

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Looks like this is still a thing. IDB is using the low level JS APIs, which no other DOM API use.