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

ServiceWorker: restructure Job Queue, Register flow, to better match spec #26317

Merged
merged 2 commits into from May 21, 2020
Merged
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

Next

serviceworker: make job queue unique per origin

  • Loading branch information
gterzian committed May 21, 2020
commit 89eb7c2aa2a949ac851ea33382af3c54efdcbc17
@@ -153,14 +153,12 @@ use script_traits::{
IFrameLoadInfo, IFrameLoadInfoWithData, IFrameSandboxState, TimerSchedulerMsg,
};
use script_traits::{
LayoutMsg as FromLayoutMsg, ScriptMsg as FromScriptMsg, ScriptThreadFactory,
Job, LayoutMsg as FromLayoutMsg, ScriptMsg as FromScriptMsg, ScriptThreadFactory,
ServiceWorkerManagerFactory,
};
use script_traits::{MediaSessionActionType, MouseEventType};
use script_traits::{MessagePortMsg, PortMessageTask, StructuredSerializedData};
use script_traits::{
SWManagerMsg, SWManagerSenders, ScopeThings, UpdatePipelineIdReason, WebDriverCommandMsg,
};
use script_traits::{SWManagerMsg, SWManagerSenders, UpdatePipelineIdReason, WebDriverCommandMsg};
use serde::{Deserialize, Serialize};
use servo_config::{opts, pref};
use servo_rand::{random, Rng, ServoRng, SliceRandom};
@@ -1983,8 +1981,8 @@ where
);
}
},
FromScriptMsg::RegisterServiceWorker(scope_things, scope) => {
self.handle_register_serviceworker(scope_things, scope);
FromScriptMsg::ScheduleJob(job) => {
self.handle_schedule_serviceworker_job(source_pipeline_id, job);
},
FromScriptMsg::ForwardDOMMessage(msg_vec, scope_url) => {
if let Some(mgr) = self.sw_managers.get(&scope_url.origin()) {
@@ -2640,9 +2638,26 @@ where
}
}

fn handle_register_serviceworker(&mut self, scope_things: ScopeThings, scope: ServoUrl) {
/// <https://w3c.github.io/ServiceWorker/#schedule-job-algorithm>
/// and
/// <https://w3c.github.io/ServiceWorker/#dfn-job-queue>
///
/// The Job Queue is essentially the channel to a SW manager,
/// which are scoped per origin.
fn handle_schedule_serviceworker_job(&mut self, pipeline_id: PipelineId, job: Job) {
let origin = job.scope_url.origin();

if self
.check_origin_against_pipeline(&pipeline_id, &origin)
.is_err()
{
return warn!(
"Attempt to schedule a serviceworker job from an origin not matching the origin of the job."

This comment has been minimized.

Copy link
@asajeffrey

asajeffrey May 11, 2020

Member

Is there a spec link for this origin check? The spec links above don't talk about origin checking.

This comment has been minimized.

Copy link
@gterzian

gterzian May 11, 2020

Author Member

Yes you're right it's not in the Spec, the idea is just to check that the origin as stored in the job message matches what the constellation has stored for the pipeline from which the message originate(it's still only limited as a check and there is a note in the function used to #11722)

);
}

// This match is equivalent to Entry.or_insert_with but allows for early return.
let sw_manager = match self.sw_managers.entry(scope.origin()) {
let sw_manager = match self.sw_managers.entry(origin.clone()) {
Entry::Occupied(entry) => entry.into_mut(),
Entry::Vacant(entry) => {
let (own_sender, receiver) = ipc::channel().expect("Failed to create IPC channel!");
@@ -2653,7 +2668,7 @@ where
own_sender: own_sender.clone(),
receiver,
};
let content = ServiceWorkerUnprivilegedContent::new(sw_senders, scope.origin());
let content = ServiceWorkerUnprivilegedContent::new(sw_senders, origin);

if opts::multiprocess() {
if content.spawn_multiprocess().is_err() {
@@ -2665,7 +2680,7 @@ where
entry.insert(own_sender)
},
};
let _ = sw_manager.send(ServiceWorkerMsg::RegisterServiceWorker(scope_things, scope));
let _ = sw_manager.send(ServiceWorkerMsg::ScheduleJob(job));
}

fn handle_broadcast_storage_event(
@@ -178,6 +178,20 @@ impl PipelineNamespace {
}
}

fn next_service_worker_id(&mut self) -> ServiceWorkerId {
ServiceWorkerId {
namespace_id: self.id,
index: ServiceWorkerIndex(self.next_index()),
}
}

fn next_service_worker_registration_id(&mut self) -> ServiceWorkerRegistrationId {
ServiceWorkerRegistrationId {
namespace_id: self.id,
index: ServiceWorkerRegistrationIndex(self.next_index()),
}
}

fn next_blob_id(&mut self) -> BlobId {
BlobId {
namespace_id: self.id,
@@ -423,6 +437,74 @@ impl fmt::Display for BroadcastChannelRouterId {
}
}

#[derive(Clone, Copy, Debug, Deserialize, Eq, Hash, Ord, PartialEq, PartialOrd, Serialize)]
pub struct ServiceWorkerIndex(pub NonZeroU32);
malloc_size_of_is_0!(ServiceWorkerIndex);

#[derive(
Clone, Copy, Debug, Deserialize, Eq, Hash, MallocSizeOf, Ord, PartialEq, PartialOrd, Serialize,
)]
pub struct ServiceWorkerId {
pub namespace_id: PipelineNamespaceId,
pub index: ServiceWorkerIndex,
}

impl ServiceWorkerId {
pub fn new() -> ServiceWorkerId {
PIPELINE_NAMESPACE.with(|tls| {
let mut namespace = tls.get().expect("No namespace set for this thread!");
let next_service_worker_id = namespace.next_service_worker_id();
tls.set(Some(namespace));
next_service_worker_id
})
}
}

impl fmt::Display for ServiceWorkerId {
fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
let PipelineNamespaceId(namespace_id) = self.namespace_id;
let ServiceWorkerIndex(index) = self.index;
write!(fmt, "(ServiceWorkerId{},{})", namespace_id, index.get())
}
}

#[derive(Clone, Copy, Debug, Deserialize, Eq, Hash, Ord, PartialEq, PartialOrd, Serialize)]
pub struct ServiceWorkerRegistrationIndex(pub NonZeroU32);
malloc_size_of_is_0!(ServiceWorkerRegistrationIndex);

#[derive(
Clone, Copy, Debug, Deserialize, Eq, Hash, MallocSizeOf, Ord, PartialEq, PartialOrd, Serialize,
)]
pub struct ServiceWorkerRegistrationId {
pub namespace_id: PipelineNamespaceId,
pub index: ServiceWorkerRegistrationIndex,
}

impl ServiceWorkerRegistrationId {
pub fn new() -> ServiceWorkerRegistrationId {
PIPELINE_NAMESPACE.with(|tls| {
let mut namespace = tls.get().expect("No namespace set for this thread!");
let next_service_worker_registration_id =
namespace.next_service_worker_registration_id();
tls.set(Some(namespace));
next_service_worker_registration_id
})
}
}

impl fmt::Display for ServiceWorkerRegistrationId {
fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
let PipelineNamespaceId(namespace_id) = self.namespace_id;
let ServiceWorkerRegistrationIndex(index) = self.index;
write!(
fmt,
"(ServiceWorkerRegistrationId{},{})",
namespace_id,
index.get()
)
}
}

#[derive(Clone, Copy, Debug, Deserialize, Eq, Hash, Ord, PartialEq, PartialOrd, Serialize)]
pub struct BlobIndex(pub NonZeroU32);
malloc_size_of_is_0!(BlobIndex);
@@ -86,6 +86,7 @@ use msg::constellation_msg::{
BlobId, BroadcastChannelRouterId, BrowsingContextId, HistoryStateId, MessagePortId,
MessagePortRouterId, PipelineId, TopLevelBrowsingContextId,
};
use msg::constellation_msg::{ServiceWorkerId, ServiceWorkerRegistrationId};
use net_traits::filemanager_thread::RelativePos;
use net_traits::image::base::{Image, ImageMetadata};
use net_traits::image_cache::{ImageCache, PendingImageId};
@@ -179,6 +180,9 @@ unsafe_no_jsmanaged_fields!(MessagePortImpl);
unsafe_no_jsmanaged_fields!(MessagePortId);
unsafe_no_jsmanaged_fields!(MessagePortRouterId);

unsafe_no_jsmanaged_fields!(ServiceWorkerId);
unsafe_no_jsmanaged_fields!(ServiceWorkerRegistrationId);

unsafe_no_jsmanaged_fields!(BroadcastChannelRouterId);

unsafe_no_jsmanaged_fields!(BlobId);
@@ -47,6 +47,7 @@ impl Client {
self.active_worker.get()
}

#[allow(dead_code)]

This comment has been minimized.

Copy link
@gterzian

gterzian May 1, 2020

Author Member

Note: this, or a variant of it, will probably be used as part of implementing #26378

pub fn set_controller(&self, worker: &ServiceWorker) {
self.active_worker.set(Some(worker));
}
@@ -41,6 +41,8 @@ use crate::dom::paintworkletglobalscope::PaintWorkletGlobalScope;
use crate::dom::performance::Performance;
use crate::dom::performanceobserver::VALID_ENTRY_TYPES;
use crate::dom::promise::Promise;
use crate::dom::serviceworker::ServiceWorker;
use crate::dom::serviceworkerregistration::ServiceWorkerRegistration;
use crate::dom::window::Window;
use crate::dom::workerglobalscope::WorkerGlobalScope;
use crate::dom::workletglobalscope::WorkletGlobalScope;
@@ -81,6 +83,7 @@ use js::rust::{HandleValue, MutableHandleValue};
use js::{JSCLASS_IS_DOMJSCLASS, JSCLASS_IS_GLOBAL};
use msg::constellation_msg::{
BlobId, BroadcastChannelRouterId, MessagePortId, MessagePortRouterId, PipelineId,
ServiceWorkerId, ServiceWorkerRegistrationId,
};
use net_traits::blob_url_store::{get_blob_origin, BlobBuf};
use net_traits::filemanager_thread::{
@@ -135,6 +138,13 @@ pub struct GlobalScope {
/// The blobs managed by this global, if any.
blob_state: DomRefCell<BlobState>,

/// <https://w3c.github.io/ServiceWorker/#environment-settings-object-service-worker-registration-object-map>
registration_map:
DomRefCell<HashMap<ServiceWorkerRegistrationId, Dom<ServiceWorkerRegistration>>>,

/// <https://w3c.github.io/ServiceWorker/#environment-settings-object-service-worker-object-map>
worker_map: DomRefCell<HashMap<ServiceWorkerId, Dom<ServiceWorker>>>,

/// Pipeline id associated with this global.
pipeline_id: PipelineId,

@@ -567,6 +577,8 @@ impl GlobalScope {
blob_state: DomRefCell::new(BlobState::UnManaged),
eventtarget: EventTarget::new_inherited(),
crypto: Default::default(),
registration_map: DomRefCell::new(HashMap::new()),
worker_map: DomRefCell::new(HashMap::new()),
pipeline_id,
devtools_wants_updates: Default::default(),
console_timers: DomRefCell::new(Default::default()),
@@ -645,6 +657,72 @@ impl GlobalScope {
);
}

/// <https://w3c.github.io/ServiceWorker/#get-the-service-worker-registration-object>
pub fn get_serviceworker_registration(
&self,
script_url: &ServoUrl,
scope: &ServoUrl,
registration_id: ServiceWorkerRegistrationId,
installing_worker: Option<ServiceWorkerId>,
_waiting_worker: Option<ServiceWorkerId>,
_active_worker: Option<ServiceWorkerId>,
) -> DomRoot<ServiceWorkerRegistration> {
// Step 1
let mut registrations = self.registration_map.borrow_mut();

if let Some(registration) = registrations.get(&registration_id) {

This comment has been minimized.

Copy link
@CYBAI

CYBAI May 3, 2020

Collaborator

nit: I personally prefer using pattern matching if we need to handle both Some and None 👀 (no need to change if you think if-else is more readable 🙏)

match registrations.get(&registration_id) {
  Some(registration) => DomRoot::from_ref(&**registration),
  None => {
    let new_registration = ...
    // ...other steps...
    new_registration
  }
}
// Step 3
return DomRoot::from_ref(&**registration);
}

// Step 2.1 -> 2.5
let new_registration =
ServiceWorkerRegistration::new(self, scope.clone(), registration_id.clone());

// Step 2.6
if let Some(worker_id) = installing_worker {
let worker = self.get_serviceworker(script_url, scope, worker_id);
new_registration.set_installing(&*worker);
}

// TODO: 2.7 (waiting worker)

// TODO: 2.8 (active worker)

// Step 2.9
registrations.insert(registration_id, Dom::from_ref(&*new_registration));

// Step 3
new_registration
}

/// <https://w3c.github.io/ServiceWorker/#get-the-service-worker-object>
pub fn get_serviceworker(
&self,
script_url: &ServoUrl,
scope: &ServoUrl,
worker_id: ServiceWorkerId,
) -> DomRoot<ServiceWorker> {
// Step 1
let mut workers = self.worker_map.borrow_mut();

if let Some(worker) = workers.get(&worker_id) {
// Step 3
DomRoot::from_ref(&**worker)
} else {
// Step 2.1
// TODO: step 2.2, worker state.
let new_worker =
ServiceWorker::new(self, script_url.clone(), scope.clone(), worker_id.clone());

// Step 2.3
workers.insert(worker_id, Dom::from_ref(&*new_worker));

// Step 3
new_worker
}
}

/// Complete the transfer of a message-port.
fn complete_port_transfer(&self, port_id: MessagePortId, tasks: VecDeque<PortMessageTask>) {
let should_start = if let MessagePortState::Managed(_id, message_ports) =
@@ -46,7 +46,7 @@ impl NavigationPreloadManagerMethods for NavigationPreloadManager {
let promise = Promise::new_in_current_realm(&*self.global(), comp);

// 2.
if self.serviceworker_registration.active().is_none() {
if self.serviceworker_registration.is_active() {
promise.reject_native(&DOMException::new(
&self.global(),
DOMErrorName::InvalidStateError,
@@ -68,7 +68,7 @@ impl NavigationPreloadManagerMethods for NavigationPreloadManager {
let promise = Promise::new_in_current_realm(&*self.global(), comp);

// 2.
if self.serviceworker_registration.active().is_none() {
if self.serviceworker_registration.is_active() {
promise.reject_native(&DOMException::new(
&self.global(),
DOMErrorName::InvalidStateError,
@@ -90,7 +90,7 @@ impl NavigationPreloadManagerMethods for NavigationPreloadManager {
let promise = Promise::new_in_current_realm(&*self.global(), comp);

// 2.
if self.serviceworker_registration.active().is_none() {
if self.serviceworker_registration.is_active() {
promise.reject_native(&DOMException::new(
&self.global(),
DOMErrorName::InvalidStateError,
@@ -114,7 +114,7 @@ impl NavigationPreloadManagerMethods for NavigationPreloadManager {
let mut state = NavigationPreloadState::empty();

// 3.
if let Some(_) = self.serviceworker_registration.active() {
if self.serviceworker_registration.is_active() {
if self
.serviceworker_registration
.get_navigation_preload_enabled()
@@ -23,6 +23,7 @@ use crate::task::TaskOnce;
use dom_struct::dom_struct;
use js::jsapi::{Heap, JSObject};
use js::rust::{CustomAutoRooter, CustomAutoRooterGuard, HandleValue};
use msg::constellation_msg::ServiceWorkerId;
use script_traits::{DOMMessage, ScriptMsg};
use servo_url::ServoUrl;
use std::cell::Cell;
@@ -35,31 +36,35 @@ pub struct ServiceWorker {
script_url: DomRefCell<String>,
scope_url: ServoUrl,
state: Cell<ServiceWorkerState>,
skip_waiting: Cell<bool>,

This comment has been minimized.

Copy link
@CYBAI

CYBAI May 3, 2020

Collaborator

IIRC, this is for the skip waiting flag. Just curios, if we don't hold this flag, how can we know if it's set or not 👀?

This comment has been minimized.

Copy link
@gterzian

gterzian May 3, 2020

Author Member

It didn't seem to be used anywhere anymore, so we can re-add it when we implement the part of the spec that uses the flag.

This comment has been minimized.

Copy link
@CYBAI

CYBAI May 4, 2020

Collaborator

Indeed! This should be easy to re-add!

worker_id: ServiceWorkerId,
}

impl ServiceWorker {
fn new_inherited(script_url: &str, skip_waiting: bool, scope_url: ServoUrl) -> ServiceWorker {
fn new_inherited(
script_url: &str,
scope_url: ServoUrl,
worker_id: ServiceWorkerId,
) -> ServiceWorker {
ServiceWorker {
eventtarget: EventTarget::new_inherited(),
script_url: DomRefCell::new(String::from(script_url)),
state: Cell::new(ServiceWorkerState::Installing),
scope_url: scope_url,
skip_waiting: Cell::new(skip_waiting),
worker_id,
}
}

pub fn install_serviceworker(
pub fn new(
global: &GlobalScope,
script_url: ServoUrl,
scope_url: ServoUrl,
skip_waiting: bool,
worker_id: ServiceWorkerId,
) -> DomRoot<ServiceWorker> {
reflect_dom_object(
Box::new(ServiceWorker::new_inherited(
script_url.as_str(),
skip_waiting,
scope_url,
worker_id,
)),
global,
)
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.