Skip to content

/ servo

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

script: Make timer events e10s-safe. #8237

Closed
wants to merge 1 commit into from
Closed

script: Make timer events e10s-safe. #8237

wants to merge 1 commit into from
+90 −119

Conversation

@pcwalton
Copy link
Contributor

pcwalton commented Oct 28, 2015

Closes #8235.

r? @jdm

Review on Reviewable
@pcwalton
Copy link
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
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
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
Member

jdm commented Oct 28, 2015

@bors-servo: r+
@bors-servo
Copy link
Contributor

bors-servo commented Oct 28, 2015

📌 Commit ac618ec has been approved by jdm
@bors-servo
Copy link
Contributor

bors-servo commented Oct 28, 2015

⌛ Testing commit ac618ec with merge b9b5493...
bors-servo added a commit that referenced this pull request Oct 28, 2015
@bors-servo
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
Contributor

bors-servo commented Oct 28, 2015

💔 Test failed - linux-dev
@jdm
Copy link
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
@pcwalton pcwalton force-pushed the pcwalton:e10s-timer-events branch from ac618ec to 587717b Oct 29, 2015
@pcwalton
script: Make timer events e10s-safe.
Loading status checks…
587717b 
Closes #8235.
@pcwalton
Copy link
Contributor Author

pcwalton commented Oct 29, 2015

@bors-servo: r=jdm
@bors-servo
Copy link
Contributor

bors-servo commented Oct 29, 2015

📌 Commit 587717b has been approved by jdm
@bors-servo
Copy link
Contributor

bors-servo commented Oct 29, 2015

⌛ Testing commit 587717b with merge 8dca100...
bors-servo added a commit that referenced this pull request Oct 29, 2015
@bors-servo
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
Contributor

bors-servo commented Oct 29, 2015

💔 Test failed - gonk
@pcwalton
Copy link
Contributor Author

pcwalton commented Oct 29, 2015

No idea what the error means. @larsbergstrom ?
@jdm
Copy link
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
Contributor

metajack commented Oct 29, 2015

Isn't tidy supposed to catch that, or do we not run that check on gonk?
@jdm
Copy link
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
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
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
Contributor

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
Member

jdm commented Oct 29, 2015

Pretty sure that check wouldn't be reached if the build failed...
@metajack
Copy link
Contributor

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
Contributor

bors-servo commented Nov 3, 2015

☔ The latest upstream changes (presumably #8295) made this pull request unmergeable. Please resolve the merge conflicts.
@jdm jdm mentioned this pull request Nov 12, 2015
Merged
@jdm jdm closed this Nov 12, 2015
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
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
@pcwalton @Ms2ger @jdm @bors-servo @metajack @SimonSapin @highfive
You can’t perform that action at this time.