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

Remove SubpageId. #7792

Closed
wants to merge 3 commits into from
Closed

Remove SubpageId. #7792

wants to merge 3 commits into from

Conversation

@glennw
Copy link
Member

glennw commented Sep 29, 2015

This changes PipelineId to be a UUID and removes the concept of SubpageId.

Iframes are now able to generate their own pipeline ID during navigation which simplifies a lot of logic in other parts of the code.

Other parts of the code, such as the compositor and layout only ever deal with pipeline ids.

Review on Reviewable

This changes PipelineId to be a UUID and removes the concept of SubpageId.

Iframes are now able to generate their own pipeline ID during navigation which simplifies a lot of logic in other parts of the code.

Other parts of the code, such as the compositor and layout only ever deal with pipeline ids.
@highfive
Copy link

highfive commented Sep 29, 2015

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
  • These commits modify layout code, but no reftests are modified. Please consider adding a reftest!
@glennw
Copy link
Member Author

glennw commented Sep 29, 2015

@jdm @Ms2ger What are your thoughts on this change?

I thought I'd prototype this and I think it worked out OK. All the ref, wpt and css tests pass, and real web sites I've tested seem to work OK.

I haven't been able to test with servo-shell yet, as it's broken in other ways at the moment.

But I wanted to get your thoughts on whether this was a reasonable approach, and if so I'll do a bit more testing before I get you to review it.

@jdm
Copy link
Member

jdm commented Sep 30, 2015

I admit that this change makes me nervous due to the possibility of UUID collision, but I don't actually know how concerned I should be about that.

@bors-servo
Copy link
Contributor

bors-servo commented Sep 30, 2015

The latest upstream changes (presumably #7423) made this pull request unmergeable. Please resolve the merge conflicts.

@glennw
Copy link
Member Author

glennw commented Sep 30, 2015

@jdm
Copy link
Member

jdm commented Sep 30, 2015

True! That being said, I am also concerned about multiprocess Servo where each content process is using a different seed, which the article does call out as a separate problem.

…eate pipeline IDs.

This is currently the constellation, and each script thread that is created.

This guarantees that pipeline IDs are unique, without relying on UUIDs.

Each time a new script thread is created, the constellation passes through a new namespace identifier.
@glennw
Copy link
Member Author

glennw commented Sep 30, 2015

@jdm What about something like the last commit? PipelineId becomes a tuple of (namespace id, index) and there is a namespace per script thread (plus the constellation). I'm not sure about the naming of the structs, but the concept seems reasonable to me. Thoughts?

@Ms2ger
Copy link
Contributor

Ms2ger commented Sep 30, 2015

I don't have a good mental model of what these are used for. Could an attacker gain anything by causing collisions?

@glennw
Copy link
Member Author

glennw commented Sep 30, 2015

I'm not sure - perhaps - but I prefer the changes in the last commit anyway, which remove the reliance on UUIDs and guarantee a unique pipeline ID.

@glennw
Copy link
Member Author

glennw commented Oct 1, 2015

Removing in favour of doing this in small increments - first step is #7807

@glennw glennw closed this Oct 1, 2015
@jdm jdm mentioned this pull request Jun 9, 2016
@aneeshusa aneeshusa mentioned this pull request Jun 10, 2016
4 of 5 tasks complete
@glennw glennw deleted the glennw:fix-pipeline-id branch Dec 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.