No manual JS_ClearPendingException when StructuredCloneHelper is used.

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
4 years ago
5 months ago

People

(Reporter: baku, Assigned: baku)

Tracking

Trunk
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Posted patch clone5.patch (obsolete) — Splinter Review
No description provided.
Attachment #8639717 - Flags: review?(bugs)
Comment on attachment 8639717 [details] [diff] [review]
clone5.patch

>@@ -88,21 +88,23 @@ BroadcastChannelChild::RecvNotify(const 
>   JSContext* cx = jsapi.cx();
>   const SerializedStructuredCloneBuffer& buffer = aData.data();
>   StructuredCloneHelper cloneHelper(StructuredCloneHelper::CloningSupported,
>                                     StructuredCloneHelper::TransferringNotSupported);
> 
>   cloneHelper.BlobImpls().AppendElements(blobs);
> 
>   JS::Rooted<JS::Value> value(cx, JS::NullValue());
>-  if (buffer.dataLength &&
>-      !cloneHelper.ReadFromBuffer(mBC->GetParentObject(), cx,
>-                                  buffer.data, buffer.dataLength, &value)) {
>-    JS_ClearPendingException(cx);
>-    return false;
>+  if (buffer.dataLength) {
>+    ErrorResult rv;
>+    cloneHelper.ReadFromBuffer(mBC->GetParentObject(), cx,
>+                               buffer.data, buffer.dataLength, &value, rv);
>+    if (NS_WARN_IF(rv.Failed())) {
>+      return false;
Hmm, do we ever want to return false from this method. This is IPC thingie, where in cross process case at least
returning false means killing the process.
Attachment #8639717 - Flags: review?(bugs) → review+
Posted patch clone5.patch (obsolete) — Splinter Review
Ok, basically what I was trying to do is not possible.
JS_ClearPendingException() must be called when we don't want to propagate the exception (mainly because cx is not exposed to content).

This patch fixes all the uses of StructuredCloneHelper and the pending exceptions. Would be nice to write a comment to explain when JS_ClearPendingExcpetion must be used and when not.
Attachment #8639717 - Attachment is obsolete: true
Attachment #8640469 - Flags: review?(bugs)
Posted patch clone5.patch (obsolete) — Splinter Review
Attachment #8640469 - Attachment is obsolete: true
Attachment #8640469 - Flags: review?(bugs)
Attachment #8640491 - Flags: review?(bugs)
Comment on attachment 8640491 [details] [diff] [review]
clone5.patch

It is not clear to me why we can't deal Read case as in attachment 8639717 [details] [diff] [review].

Why SharedMessagePortMessage::Read doesn't clear the exception but other Reads do?
Attachment #8640491 - Flags: review?(bugs) → review-
> It is not clear to me why we can't deal Read case as in attachment 8639717 [details] [diff] [review]

To be consistent between Read and Write.
 
> Why SharedMessagePortMessage::Read doesn't clear the exception but other
> Reads do?

It's done in MessagePort.cpp where that ::Read is used.
Flags: needinfo?(bugs)
This patch removes the exception in the Read()s.
Attachment #8640955 - Flags: review?(bugs)
Yeah, I prefer to remove it, since exception handling is just extra code and easy to forget.
Flags: needinfo?(bugs)
Attachment #8640491 - Attachment is obsolete: true
Comment on attachment 8640955 [details] [diff] [review]
clone5.patch - 2


>+  if (buffer.dataLength) {
>+    ErrorResult rv;
>+    cloneHelper.ReadFromBuffer(mBC->GetParentObject(), cx,
>+                               buffer.data, buffer.dataLength, &value, rv);
>+    if (NS_WARN_IF(rv.Failed())) {
>+      rv.SuppressException();
No sure why we need rv.SuppressException() here. Please add a comment why or remove.
Attachment #8640955 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/639c7d0805bf
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.