Skip to content

script: Make timer events e10s-safe.#8237

Closed
pcwalton wants to merge 1 commit intoservo:masterfrom
pcwalton:e10s-timer-events
Closed

script: Make timer events e10s-safe.#8237
pcwalton wants to merge 1 commit intoservo:masterfrom
pcwalton:e10s-timer-events

Conversation

@pcwalton
Copy link
Copy Markdown
Contributor

@pcwalton pcwalton commented Oct 28, 2015

Closes #8235.

r? @jdm

Review on Reviewable

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 28, 2015
@pcwalton
Copy link
Copy Markdown
Contributor Author

pcwalton commented Oct 28, 2015

Until this lands e10s cannot land.

@pcwalton pcwalton mentioned this pull request Oct 28, 2015
Closed
@Ms2ger
Copy link
Copy Markdown
Contributor

Ms2ger commented Oct 28, 2015

Review status: 0 of 13 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

components/script/dom/dedicatedworkerglobalscope.rs, line 232 [r1] (raw file):
Does this leak all workers until process exit?

Comments from the review on Reviewable.io

@jdm
Copy link
Copy Markdown
Member

jdm commented Oct 28, 2015

This should not change the lifetime of workers from the status quo (which will, in fact, leak until the ActiveTimers value is dropped). Addressing that issue should happen separately.

@jdm
Copy link
Copy Markdown
Member

jdm commented Oct 28, 2015

@bors-servo: r+

@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Oct 28, 2015

📌 Commit ac618ec 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 Oct 28, 2015
@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Oct 28, 2015

⌛ Testing commit ac618ec with merge b9b5493...

bors-servo pushed a commit that referenced this pull request Oct 28, 2015
Auto merge of #8237 - pcwalton:e10s-timer-events, r=jdm
b9b5493 
script: Make timer events e10s-safe.

Closes #8235.

r? @jdm

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8237)
<!-- Reviewable:end -->
@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Oct 28, 2015

💔 Test failed - linux-dev

@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 Oct 28, 2015
@jdm
Copy link
Copy Markdown
Member

jdm commented Oct 28, 2015 

./ports/gonk/Cargo.lock:259: conflicting versions for package "serde_macros"
    expected maximum version "0.6.1"
    but, "compositing" demands "0.5.3"
    try upgrading with ./mach cargo-update -p serde_macros:0.5.3

@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Oct 29, 2015
@pcwalton
Copy link
Copy Markdown
Contributor Author

pcwalton commented Oct 29, 2015

@bors-servo: r=jdm

@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Oct 29, 2015

📌 Commit 587717b 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 Oct 29, 2015
@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Oct 29, 2015

⌛ Testing commit 587717b with merge 8dca100...

bors-servo pushed a commit that referenced this pull request Oct 29, 2015
Auto merge of #8237 - pcwalton:e10s-timer-events, r=jdm
8dca100 
script: Make timer events e10s-safe.

Closes #8235.

r? @jdm

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8237)
<!-- Reviewable:end -->
@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Oct 29, 2015

💔 Test failed - gonk

@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 Oct 29, 2015
@pcwalton
Copy link
Copy Markdown
Contributor Author

pcwalton commented Oct 29, 2015

No idea what the error means. @larsbergstrom ?

@jdm
Copy link
Copy Markdown
Member

jdm commented Oct 29, 2015

Plausibly the gonk Cargo.lock isn't up to date, so it ends up pulling in the latest version of something that isn't specified?

@metajack
Copy link
Copy Markdown
Collaborator

metajack commented Oct 29, 2015

Isn't tidy supposed to catch that, or do we not run that check on gonk?

@jdm
Copy link
Copy Markdown
Member

jdm commented Oct 29, 2015

Tidy only looks for incompatible entries in single lock files; there is no inter-file comparison.

@jdm
Copy link
Copy Markdown
Member

jdm commented Oct 29, 2015

I just checked and the lock files are fine on master, and I can't see why this PR would affect that since no toml files are modified, however :(

@jdm
Copy link
Copy Markdown
Member

jdm commented Oct 29, 2015

Oh wait, there are Cargo.lock file changes only for gonk that upgrade serde, which presumably is incompatible with the version of rustc we're currently using.

@metajack
Copy link
Copy Markdown
Collaborator

metajack commented Oct 29, 2015

@jdm I meant that if the lockfile after build is different than the lockfile it started with. That is supposed to catch out of date Cargo.locks

@jdm
Copy link
Copy Markdown
Member

jdm commented Oct 29, 2015

Pretty sure that check wouldn't be reached if the build failed...

@metajack
Copy link
Copy Markdown
Collaborator

metajack commented Oct 29, 2015

@jdm Well if the lockfile was out of date, then there was a build that passed with it.

@pcwalton Can you fix this up?

@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Nov 3, 2015

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

@highfive highfive added S-needs-rebase There are merge conflict errors. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Nov 3, 2015
@jdm jdm mentioned this pull request Nov 12, 2015
Merged
@jdm jdm closed this Nov 12, 2015
@SimonSapin SimonSapin removed the S-tests-failed The changes caused existing tests to fail. label Jan 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@jdm jdm

Labels

S-needs-rebase There are merge conflict errors.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@pcwalton @Ms2ger @jdm @bors-servo @metajack @SimonSapin @highfive