Skip to content

Introduce a new type MaybeUnreflectedDom<T> (fixes #25701)#25781

Merged
bors-servo merged 1 commit intoservo:masterfrom
nox:fixzeal
Mar 2, 2020
Merged

Introduce a new type MaybeUnreflectedDom<T> (fixes #25701)#25781
bors-servo merged 1 commit intoservo:masterfrom
nox:fixzeal

Conversation

@nox
Copy link
Copy Markdown
Contributor

@nox nox commented Feb 17, 2020

No description provided.

@highfive
Copy link
Copy Markdown

highfive commented Feb 17, 2020

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/bindings/codegen/CodegenRust.py, components/script/lib.rs, components/script/dom/windowproxy.rs, components/script/dom/bindings/reflector.rs, components/script/dom/bindings/root.rs and 2 more
  • @KiChjang: components/script/dom/bindings/codegen/CodegenRust.py, components/script/lib.rs, components/script/dom/windowproxy.rs, components/script/dom/bindings/reflector.rs, components/script/dom/bindings/root.rs and 2 more

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Feb 17, 2020
@highfive
Copy link
Copy Markdown

highfive commented Feb 17, 2020

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
  • These commits modify script code, but no tests are modified. Please consider adding a test!

@nox
Copy link
Copy Markdown
Contributor Author

nox commented Feb 17, 2020

@bors-servo try=wpt

@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Feb 17, 2020

⌛ Trying commit 14846d0 with merge 4bf1aa0...

bors-servo pushed a commit that referenced this pull request Feb 17, 2020
Auto merge of #25781 - nox:fixzeal, r=<try>
4bf1aa0 
Introduce a new type MaybeUnreflectedDom<T> (fixes #25701)
@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Feb 17, 2020

💔 Test failed - status-taskcluster

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label Feb 17, 2020
@nox
Copy link
Copy Markdown
Contributor Author

nox commented Feb 19, 2020

All the failures are either the solid white intermittent issue or a silent crash that happens sometimes.

@nox
Copy link
Copy Markdown
Contributor Author

nox commented Feb 19, 2020

That silent crash doesn't even reproduce locally.

@jdm
Copy link
Copy Markdown
Member

jdm commented Feb 21, 2020

Could you give me an overview of what this PR is doing? It's not clear to me how all the pieces fit together to fix #25701.

@jdm jdm added S-awaiting-answer Someone asked a question that requires an answer. and removed S-awaiting-review There is new code that needs to be reviewed. S-tests-failed The changes caused existing tests to fail. labels Feb 21, 2020
@nox
Copy link
Copy Markdown
Contributor Author

nox commented Feb 21, 2020

Could you give me an overview of what this PR is doing? It's not clear to me how all the pieces fit together to fix #25701.

Sure. What it does is that it roots the boxed not-yet-reflected DOM instance with a different smart pointer type than Dom<T>. MaybeUnreflectedDom<T> traces the rooted T by first checking whether the reflector is set, in which case it proceeds just like Dom<T>, otherwise it directly traces the T, which will invoke the JSTraceable implementation of Reflector, which will do nothing but that's not a problem given it's not set yet.

@jdm jdm added S-awaiting-review There is new code that needs to be reviewed. and removed S-awaiting-answer Someone asked a question that requires an answer. labels Feb 24, 2020
jdm
jdm approved these changes Feb 28, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@jdm jdm left a comment

Choose a reason for hiding this comment

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

This is a nice solution to the problem! Thanks for explaining it to me.

@jdm
Copy link
Copy Markdown
Member

jdm commented Feb 28, 2020

@bors-servo r+

@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Feb 28, 2020

📌 Commit 14846d0 has been approved by jdm

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Feb 28, 2020
@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Feb 28, 2020

⌛ Testing commit 14846d0 with merge b6ecb05...

bors-servo pushed a commit that referenced this pull request Feb 28, 2020
Auto merge of #25781 - nox:fixzeal, r=jdm
b6ecb05 
Introduce a new type MaybeUnreflectedDom<T> (fixes #25701)
@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Feb 28, 2020

💔 Test failed - status-taskcluster

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Feb 28, 2020
@nox
Copy link
Copy Markdown
Contributor Author

nox commented Feb 28, 2020

Not sure if this crash is my fault or not, we'll see how the other build jobs fare.

@asajeffrey
Copy link
Copy Markdown
Contributor

asajeffrey commented Feb 28, 2020

This is getting us pretty close to being able to support lazy creation of DOM reflectors.

@nox
Copy link
Copy Markdown
Contributor Author

nox commented Feb 28, 2020

This only works because the T here is always the actual concrete type of the DOM instance, if you create an Element and you store it as MaybeUnreflectedDom<Node>, you won't trace everything that is specific to Element.

@asajeffrey
Copy link
Copy Markdown
Contributor

asajeffrey commented Feb 28, 2020

Hmm, so we'd need support for dynamic dispatch of trace methods if we want to support subtyping of maybe-reflected DOM objects.

@nox
Copy link
Copy Markdown
Contributor Author

nox commented Mar 2, 2020

Tried to reproduce the issue locally and failed.

@bors-servo retry

@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Mar 2, 2020

⌛ Testing commit 14846d0 with merge 554c90a...

bors-servo pushed a commit that referenced this pull request Mar 2, 2020
Auto merge of #25781 - nox:fixzeal, r=jdm
554c90a 
Introduce a new type MaybeUnreflectedDom<T> (fixes #25701)
@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Mar 2, 2020
@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Mar 2, 2020

💔 Test failed - status-taskcluster

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Mar 2, 2020
@jdm
Copy link
Copy Markdown
Member

jdm commented Mar 2, 2020

@bors-servo retry
Network

@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Mar 2, 2020

⌛ Testing commit 14846d0 with merge 5a6b2d9...

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Mar 2, 2020
@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Mar 2, 2020

☀️ Test successful - status-taskcluster
Approved by: jdm
Pushing 5a6b2d9 to master...

@bors-servo bors-servo merged commit 5a6b2d9 into servo:master Mar 2, 2020
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Mar 2, 2020
This was referenced Mar 2, 2020
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jdm jdm jdm approved these changes

Assignees

@jdm jdm

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@nox @highfive @bors-servo @jdm @asajeffrey