Closed
Bug 1188265
Opened 9 years ago
Closed 9 years ago
No manual JS_ClearPendingException when StructuredCloneHelper is used.
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla42
Tracking | Status | |
---|---|---|
firefox42 | --- | fixed |
People
(Reporter: baku, Assigned: baku)
References
Details
Attachments
(1 file, 3 obsolete files)
17.00 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Attachment #8639717 -
Flags: review?(bugs)
Comment 1•9 years ago
|
||
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+
Assignee | ||
Comment 2•9 years ago
|
||
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)
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8640469 -
Attachment is obsolete: true
Attachment #8640469 -
Flags: review?(bugs)
Attachment #8640491 -
Flags: review?(bugs)
Comment 4•9 years ago
|
||
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-
Assignee | ||
Comment 5•9 years ago
|
||
> 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)
Assignee | ||
Comment 6•9 years ago
|
||
This patch removes the exception in the Read()s.
Attachment #8640955 -
Flags: review?(bugs)
Comment 7•9 years ago
|
||
Yeah, I prefer to remove it, since exception handling is just extra code and easy to forget.
Flags: needinfo?(bugs)
Assignee | ||
Updated•9 years ago
|
Attachment #8640491 -
Attachment is obsolete: true
Comment 8•9 years ago
|
||
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+
Assignee | ||
Comment 9•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/639c7d0805bf
Comment 10•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/639c7d0805bf
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•