Implement structured cloning of DataViews

RESOLVED FIXED in Firefox 43

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: sfink, Assigned: sfink)

Tracking

unspecified
mozilla43
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox43 fixed)

Details

(Whiteboard: [js:t])

Attachments

(1 attachment)

The current implementation does not allow cloning DataViews. Bug 789593 should be implemented before this bug, though.
Depends on: 789593
Whiteboard: [js:t]
Assignee: general → nobody

Updated

4 years ago
Duplicate of this bug: 819838
Created attachment 8640255 [details] [diff] [review]
Implement DataView cloning

Does what it says on the tin.
Attachment #8640255 - Flags: review?(jwalden+bmo)
Assignee: nobody → sphink
Status: NEW → ASSIGNED
Depends on: 789589
Blocks: 928003

Comment 4

3 years ago
To be clear, it should also allow the underlying buffer to be transferred.

var worker = new Worker('test.js');
var buffer = new ArrayBuffer(10);
var dataView = new DataView(buffer);
worker.postMessage(dataView, [buffer]);

Comment 5

3 years ago
Comment on attachment 8640255 [details] [diff] [review]
Implement DataView cloning

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

Come one, come all, get your cargo-cult reviews here!  Nothing but the finest in hackish comparisons against similar cases in existing code, backed only by handwavy understanding of the approaches being implemented, without deep grokking sufficient to be fully confident of correctness!  The greatest farce in SpiderMonkey!

...okay, not quite that bad, but really all I did was compare what you did to what's done now for typed arrays, without real familiarity with that code or deep understanding of the structured clone implementation techniques.  :-)

::: js/src/gdb/mozilla/prettyprinters.py
@@ +173,5 @@
>          self.void_t = gdb.lookup_type('void')
>          self.void_ptr_t = self.void_t.pointer()
>          try:
>              self.JSString_ptr_t = gdb.lookup_type('JSString').pointer()
> +            # self.JSSymbol_ptr_t = gdb.lookup_type('JS::Symbol').pointer()

This seems unrelated?

::: js/src/vm/StructuredClone.cpp
@@ +1446,5 @@
> +    // Read byteOffset.
> +    uint64_t n;
> +    if (!in.read(&n))
> +        return false;
> +    uint32_t byteOffset = n;

Use mozilla::AssertedCast<uint32_t>(n) from #include "mozilla/Casting.h" here.  (Bonus points for a patch doing that everywhere in this file, actually.)
Attachment #8640255 - Flags: review?(jwalden+bmo) → review+

Comment 6

3 years ago
(In reply to Sebastian Markbåge from comment #4)
> To be clear, it should also allow the underlying buffer to be transferred.

Assuming the existing typed array structured cloning code does that -- and I'm 99.99999% sure it does -- the cargo-culted code in this patch should do so as well, never fear.
https://hg.mozilla.org/mozilla-central/rev/65b1a0d77304
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.