XHR timeouts use same abstraction as scripts timers. (fixes #3396) #8168

benschulz · 2015-10-23T09:59:30Z

Alright, this is it. Finally the fix for #3396. :D

I'll add two comments via reviewable in a second.

benschulz · 2015-10-23T10:07:18Z

Review status: 0 of 6 files reviewed at latest revision, 3 unresolved discussions, some commit checks pending.

^{components/script/dom/xmlhttprequest.rs, line 963 [r1] (raw file):}
Actually I still need a Trusted later (to send the RunnableMsg). Still, I'd rather not add HeapSizeOf for Trusted. Should I use a JS<XMLHttpRequest>?

^{components/script/timers.rs, line 124 [r1] (raw file):}
I feel like this entire trait isn't "rusty".

^{components/script/timers.rs, line 189 [r1] (raw file):}
I don't like that setTimeout/setInterval and XMLHttpRequest are getting this complected. I'd like to file another PR to extract a minimal abstraction based on ScheduledCallback. XHR timeouts and (the old) ActiveTimers would sit on top of that abstraction. Thoughts?

Comments from the review on Reviewable.io

nox · 2015-10-23T12:25:39Z

Not a proper review, but some questions and a couple of doc nits.

+S-needs-code-changes

Review status: 0 of 6 files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.

^{components/script/dom/bindings/global.rs, line 201 [r1] (raw file):}
"Schedule"

^{components/script/dom/bindings/global.rs, line 210 [r1] (raw file):}
"Unschedule a previously-scheduled callback."

^{components/script/dom/xmlhttprequest.rs, line 963 [r1] (raw file):}
I'm pretty sure the XMLHttpRequest value isn't owned by ScheduledXHRTimeout, so its HeapSizeOf impl should ignore it anyway. Same for the target field, actually.

^{components/script/dom/xmlhttprequest.rs, line 969 [r1] (raw file):}
Why the explicit deref?

^{components/script/timers.rs, line 124 [r1] (raw file):}
Given Box<T> derefs to T, couldn't you just have a trait like this?

pub trait ScheduledCallback: Clone + JSTraceable + HeapSizeOf {
    fn invoke(self);
}

Comments from the review on Reviewable.io

benschulz · 2015-10-23T13:05:51Z

Thanks for the input. =)

Review status: 0 of 6 files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

^{components/script/dom/xmlhttprequest.rs, line 963 [r1] (raw file):}
But shouldn't the overhead of the Trusted pointer be counted?

^{components/script/dom/xmlhttprequest.rs, line 969 [r1] (raw file):}
Otherwise I'd have to clone xhr, wouldn't I? (Not a rhetorical question. :D)

^{components/script/timers.rs, line 124 [r1] (raw file):}
I tried that, but deriving Clone ended up not working. It's somewhat odd given impl Clone for Box<T> where T: Clone. There were similar errors for implementors of ScheduledCallback.

Comments from the review on Reviewable.io

eefriedman · 2015-10-23T22:10:28Z

Review status: 0 of 6 files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

^{components/script/timers.rs, line 124 [r1] (raw file):}
The Clone trait isn't object-safe; you can't write Box<Clone>, or any variation.

At a slightly higher level, this shouldn't really need to implement clone: a oneshot timer can only execute once, so the callback never actually needs to be cloned. It should be possible to encode this into the type system.

Comments from the review on Reviewable.io

benschulz · 2015-10-24T05:08:50Z

Review status: 0 of 6 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

^{components/script/timers.rs, line 124 [r1] (raw file):}

The Clone trait isn't object-safe; you can't write Box, or any variation.

Thanks, this confirms my suspcicion. Also, I just realized that impl<T> Clone for Box<T> where T: Cloneisn't applicable due to the implicit Sized constraint. I'm still getting the hang of rust.

At a slightly higher level, this shouldn't really need to implement clone

The cloning is currently necessary because InternalTimerCallbacks need to be cloneable. I'd need to introduce a separate abstraction (see other discussion) to get rid of that requirement on ScheduledCallback. Perhaps I should fold it into this PR though…

Comments from the review on Reviewable.io

bors-servo · 2015-10-25T01:20:06Z

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

jdm · 2015-11-10T21:11:41Z

-S-awaiting-review +S-needs-code-changes

Reviewed 6 of 6 files at r1.
Review status: 5 of 6 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

^{components/script/dom/xmlhttprequest.rs, line 963 [r1] (raw file):}
Trusted is necessary for sending JS pointers between threads; JS<T> isn't safe since it could be garbage collected. As for the overhead of the Trusted value vs. its pointed-at contents, that's already accounted for by the heap profiling code.

^{components/script/dom/xmlhttprequest.rs, line 969 [r1] (raw file):}
I don't see why. The Box<Self> syntax allow us to move out of self at will, same as in the various implementations of Runnable.

^{components/script/timers.rs, line 124 [r1] (raw file):}
@benschulz: I threw together a diff which eliminates the Clone requirement for InternalTimerCallback. Maybe that helps?

^{components/script/timers.rs, line 189 [r1] (raw file):}
I like that idea in principle. I agree that there's a surprising amount of machinery here for these things.

Comments from the review on Reviewable.io

jdm · 2015-11-10T21:11:58Z

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

Comments from the review on Reviewable.io

benschulz · 2015-11-10T22:31:05Z

Review status: 5 of 6 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

^{components/script/dom/xmlhttprequest.rs, line 963 [r1] (raw file):}
My reasoning to remove the Trusted was that the XMLHttpRequest isn't leaving the script task anymore. My reasoning for a JS pointer was, that it should be reachable by the GC through ActiveTimers. However, the intricacies of the various pointer types still escape me. So if you say, Trusted is the way to go, then I'll trust you. ;)

(If there's a reference explaining the pointer types in more detail than Using DOM types, I'd appreciate a link.)

^{components/script/dom/xmlhttprequest.rs, line 969 [r1] (raw file):}
My understand is that I can't move out of s while s.target.send(… borrows it. This is the most compact and direct way arround that, I came up with.

^{components/script/timers.rs, line 124 [r1] (raw file):}
@jdm That's roughly what it used to look like, but it breaks a test. Essentially the repeating timer needs to be rescheduled before its callback is executed, so that it can unschedule itself. I don't see any way of getting rid of Clone before the other refactoring.

^{components/script/timers.rs, line 189 [r1] (raw file):}
Should it be part of this PR, or is it okay if I file a separate one?

Comments from the review on Reviewable.io

jdm · 2015-11-10T23:39:09Z

Let's rebase and squash this :)
-S-awaiting-review

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

^{components/script/dom/xmlhttprequest.rs, line 963 [r1] (raw file):}
Oh, right. You are correct about the reachability, but the use of JS<T> would be harder to prove correct for the compiler in this case due to the whole boxing thing, so let's leave it as Trusted. I don't know of a better reference than one the you linked, but feel free to file an issue (or use the mailing list) to list things you find confusing or incomplete about the current docs!

^{components/script/dom/xmlhttprequest.rs, line 969 [r1] (raw file):}
Fascinating; https://play.rust-lang.org/?gist=f03483eab3bd1c6b133e&version=stable agrees with you.

^{components/script/timers.rs, line 124 [r1] (raw file):}
Womp womp.

^{components/script/timers.rs, line 189 [r1] (raw file):}
Separate is fine with me.

Comments from the review on Reviewable.io


        XHR timeouts use same abstraction as scripts timers. (fixes #3396)

jdm · 2015-11-11T00:00:08Z

@bors-servo: r+

bors-servo · 2015-11-11T00:00:09Z

📌 Commit d27a324 has been approved by jdm

bors-servo · 2015-11-11T00:38:29Z

⌛ Testing commit d27a324 with merge df81cd7...


        Auto merge of #8168 - benschulz:xhr-timeout-ordering, r=jdm

XHR timeouts use same abstraction as scripts timers. (fixes #3396) Alright, this is it. Finally the fix for #3396. :D I'll add two comments via reviewable in a second.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8168)

bors-servo · 2015-11-11T01:25:38Z

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt


        Auto merge of #8475 - benschulz:xhr-timeout-ordering2, r=jdm

Execute XHR timeout callbacks directly. (Fixes #8468.) This is a fix for #8468. Currently XHR timeouts schedule themselves for execution via `CommonScriptMsg::RunnableMsg`s. This was necessary when these timeouts used a separate thread to schedule themselves. Now it's a potential race that should have been eliminated as part of #8168.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8475)

highfive added the S-awaiting-review label Oct 23, 2015

nox added the S-needs-code-changes label Oct 23, 2015

jdm self-assigned this Oct 23, 2015

highfive removed the S-needs-code-changes label Oct 23, 2015

highfive added S-needs-rebase and removed S-awaiting-merge labels Oct 25, 2015

jdm added S-needs-code-changes and removed S-awaiting-review labels Nov 10, 2015

highfive added S-awaiting-review and removed S-needs-code-changes labels Nov 10, 2015

jdm removed the S-awaiting-review label Nov 10, 2015

XHR timeouts use same abstraction as scripts timers. (fixes #3396)

Loading status checks…

d27a324

benschulz force-pushed the benschulz:xhr-timeout-ordering branch from 0b1814d to d27a324 Nov 10, 2015

highfive added the S-awaiting-review label Nov 10, 2015

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Nov 11, 2015

benschulz mentioned this pull request Nov 11, 2015

Execute XHR timeout callbacks directly. (Fixes #8468.) #8475

Merged

servo / servo

XHR timeouts use same abstraction as scripts timers. (fixes #3396) #8168

XHR timeouts use same abstraction as scripts timers. (fixes #3396) #8168

benschulz commented Oct 23, 2015

benschulz commented Oct 23, 2015

nox commented Oct 23, 2015

benschulz commented Oct 23, 2015

eefriedman commented Oct 23, 2015

benschulz commented Oct 24, 2015

bors-servo commented Oct 25, 2015

jdm commented Nov 10, 2015

jdm commented Nov 10, 2015

benschulz commented Nov 10, 2015

jdm commented Nov 10, 2015

jdm commented Nov 11, 2015

bors-servo commented Nov 11, 2015

bors-servo commented Nov 11, 2015

bors-servo commented Nov 11, 2015

servo / servo

Join GitHub today

XHR timeouts use same abstraction as scripts timers. (fixes #3396) #8168

XHR timeouts use same abstraction as scripts timers. (fixes #3396) #8168

Conversation

benschulz commented Oct 23, 2015

benschulz commented Oct 23, 2015

nox commented Oct 23, 2015

benschulz commented Oct 23, 2015

eefriedman commented Oct 23, 2015

benschulz commented Oct 24, 2015

bors-servo commented Oct 25, 2015

jdm commented Nov 10, 2015

jdm commented Nov 10, 2015

benschulz commented Nov 10, 2015

jdm commented Nov 10, 2015

jdm commented Nov 11, 2015

bors-servo commented Nov 11, 2015

bors-servo commented Nov 11, 2015

bors-servo commented Nov 11, 2015