Closed Bug 1188265 Opened 5 years ago Closed 5 years ago

No manual JS_ClearPendingException when StructuredCloneHelper is used.

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: baku, Assigned: baku)

References

Details

Attachments

(1 file, 3 obsolete files)

Attached 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+
Attached 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)
Attached 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)
Attached patch clone5.patch - 2Splinter Review
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: 5 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.