Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix issue #128 copy buffer sources #130

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

equalsJeffH
Copy link
Collaborator

@equalsJeffH equalsJeffH commented Oct 23, 2018

fixes #128

this PR employs the proposed <dfn>create a copy of any buffer sources' data</dfn> alg from #128 (comment)

see also: https://lists.w3.org/Archives/Public/public-webappsec/2018Oct/0027.html and https://lists.w3.org/Archives/Public/public-webappsec/2018Oct/0032.html


Preview | Diff

@equalsJeffH equalsJeffH self-assigned this Oct 23, 2018
Copy link

@jcjones jcjones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

[=get a copy of the bytes held by the buffer source=] and
set |values|'s value to be a reference to the copy of the bytes.

issue: do we need to explicitly free the bytes we just orphaned?
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bytes that belong to |V| [|member|] aren't orphaned, as they are still rooted to |dictionary|. The return could get orphaned, but that's not this algorithm's fault.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was only referring to the buffer source bytes that we copied from and then overwrote the dictionary's references to them -- do we just assume there is garbage collection?


2. If |value|'s type is a [=buffer source type=] then
[=get a copy of the bytes held by the buffer source=] and
set |values|'s value to be a reference to the copy of the bytes.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guessing you meant:

Suggested change
set |values|'s value to be a reference to the copy of the bytes.
set |value|'s value to be a reference to the copy of the bytes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOL!! yes, thanks :)

Copy link
Member

@mikewest mikewest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I didn't get to this last week. I'm getting to it now!

[=dictionary=] type, run the following steps given |V|:

<ol class="algorithm">
1. Let |dictionaries| be a list consisting of |V| and all of |V|'s
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very Important Style Nit: The rest of the doc uses spaces, not tabs. :)

@@ -548,6 +548,34 @@ <h1>Credential Management Level 1</h1>
to guide the discovery process (by choosing a federated identity provider, BTLE device, etc).
</div>

<div algorithm="copy any buffer sources">
To <dfn>create a copy of any buffer sources' data</dfn> referenced by a value |V| of
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following reads a little more clearly to me:

Suggested change
To <dfn>create a copy of any buffer sources' data</dfn> referenced by a value |V| of
To <dfn>create a copy of any buffer sources' data</dfn> referenced by a [=dictionary=] value |V|, run ...


<ol class="algorithm">
1. Let |dictionaries| be a list consisting of |V| and all of |V|'s
[=inherited dictionaries=], in order from least to most derived.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need to walk through the inherited dictionaries? Doesn't |V| itself already have all of the members of dictionaries from which it derives?

2. For each dictionary |dictionary| in |dictionaries|, in order:

1. For each dictionary member |member| declared on
|dictionary|, in lexicographical order:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than walking through the dictionaries, did you consider just walking through |V|'s members?

@mikewest
Copy link
Member

Ping! Let's keep this moving. :)

Base automatically changed from master to main February 16, 2021 23:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

copy (aka snapshot) any buffersources in options before going async
3 participants