Use the destination to refine the fetchpriority mapping of FetchDriver::HttpFetch
Categories
(Core :: Networking, enhancement, P2)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox129 | --- | fixed |
People
(Reporter: zsun, Assigned: fredw)
References
(Blocks 2 open bugs)
Details
(Whiteboard: [necko-triaged])
Attachments
(7 files, 1 obsolete file)
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
102.71 KB,
application/zip
|
Details |
https://fetch.spec.whatwg.org/#concept-request-destination
In https://web.dev/articles/fetch-priority, it mentions that - fetch using a specific "destination" use the priority of the destination they are requesting. We need to take "destination" cases into account and refine the fetchpriority mapping for fetch() API.
|
||
Updated•1 year ago
|
|
Assignee | |
Comment 1•9 months ago
|
||
The description from the web.dev article is:
Preload using "as" or fetch using a specific "destination" use the priority of the destination they are requesting. For example, preload as="style" will use Highest priority. With no explicit destintation, fetch will be prioritized like an async XHR.
Ziran commented to me two months ago that the determination of the destination is done here:
https://searchfox.org/mozilla-central/rev/eb4700a6be8371fe07053bc066c2d48ba813ce3d/dom/fetch/InternalRequest.cpp#263
https://searchfox.org/mozilla-central/rev/eb4700a6be8371fe07053bc066c2d48ba813ce3d/dom/fetch/InternalRequest.h#38
https://searchfox.org/mozilla-central/rev/eb4700a6be8371fe07053bc066c2d48ba813ce3d/dom/fetch/InternalRequest.h#280
She mentioned that for global fetch (nsIContentPolicy::TYPE_FETCH) the destination is always the empty string, so that's quite confusing.
After checking a bit the spec with her, the relevant sections seem to be the following ones:
Setting priority during fetch: https://fetch.spec.whatwg.org/#fetching
if request’s internal priority is null, then use request’s priority, initiator, destination, and render-blocking in an implementation-defined manner to set request’s internal priority to an implementation-defined object.
Requests: https://fetch.spec.whatwg.org/#requests
A request has an associated destination, which is the empty string, "audio", "audioworklet", "document", "embed", "font", "frame", "iframe", "image", "json", "manifest", "object", "paintworklet", "report", "script", "serviceworker", "sharedworker", "style", "track", "video", "webidentity", "worker", or "xslt". Unless stated otherwise it is the empty string.
Setting up a request: https://fetch.spec.whatwg.org/#ref-for-concept-request-destination%E2%91%A1%E2%91%A0
Choose your request’s destination using the guidance in the destination table. Destinations affect Content Security Policy and have other implications such as the
Sec-Fetch-Destheader, so they are much more than informative metadata. If a new feature requires a destination that’s not in the destination table, please file an issue to discuss. [CSP]
Destination table: https://fetch.spec.whatwg.org/#destination-table
So it seems everything is already defined in the spec and Gecko implements it. The only missing bit is taking into account mRequest.Destination() to adjust priority. But the point is that it is not just for the global fetch method, but for anything that relies on the fetch code under the hood.
Ziran found the following WPT test for rel=preload and as=... we can probably rely on:
|
|
Assignee | |
Comment 2•9 months ago
|
||
From a quick look at Chromium, it seems it does not use the request's destination but an internal resource type:
|
|
Assignee | |
Comment 3•9 months ago
|
||
I tried running fetch tests but it seems mRequest almost always use mContentPolicyType == nsIContentPolicy::TYPE_FETCH and so mRequest->Destination == RequestDestination::_empty.
So far the only exception I found is this test for audioworklet (e.g. fetch-destination.https.html, audio-worklet.https.html, ...). In general Worklet set content policy here but there don't seem to be many of them (other specs than the Web Audio API don't seem to redefine their destination currently).
Checking the code:
-
mContentPolicyTypeis set inSetContentPolicyType()(see next item), in a copy method or in a constructor. But skimming over the occurences I don't see where the latter is used with an explicit content policy type argument. -
SetContentPolicyType is only called by
overrideContentPolicyType(see next item) or when converting a CacheRequest to an InternalRequest. And the only place when a CacheRequest has its content policy type set is when converted from an internal request and here. But I'm not sure how to trigger that one? -
overrideContentPolicyType is exposed to Chrome callers which means we could probably rely on that for tests if we wish. Not sure modern extensions have access to this API though. The function is otherwise used in a few other places with in our code with TYPE_WEB_IDENTITY, TYPE_IMAGE, and TYPE_WEB_MANIFEST.
|
|
Assignee | |
Comment 4•9 months ago
|
||
|
|
Assignee | |
Comment 5•9 months ago
|
||
| Comment hidden (obsolete) |
|
||
Updated•9 months ago
|
|
|
Assignee | |
Comment 7•9 months ago
|
||
I've spent more time debugging what's happening, but the TL;DR is that a patch like attachment 9398333 [details] is unlikely to produce any observable change, because the nsIContentPolicy is not properly propagated to FetchDriver::HttpFetch so destination is almost alway empty.
Summary for existing tests:
-
In general, the
InternalRequest/FetchDriverdoes not seem to be used at all for the kind of fetchpriority things we implemented and tested inmozilla/tests/fetch/fetchpriority/(bug 1797715). This is a good thing since that means taking destination into account won't add extra priority shift on top of what we already implemented. -
The exception from the previous item is global fetch (bug 1839319), covered by mozila/tests/fetch/fetchpriority/support/fetch-tests/fetch-init.h2.html. But in that case, it seems Requests created by JS from a string URLs always use nsIContentPolicy::TYPE_FETCH, so we keep using priority for the default (empty) destination.
-
Regarding tests/fetch/api/request/destination/fetch-destination.https.html, an
nsContentPolicyTypeis set when creating a channel which is forwarded to the load info. However, when that channel is intercepted by the worker and the fetch performed on the worker's global scope, it is reset tonsIContentPolicy::TYPE_FETCHand so we end up using an empty destination. -
The exception from the previous item being the case of worklets.
WorkletFetchHandler::StartFetchwill override the content policy type so that it is preserved when the fetch is later executed. However, again when the channel is intercepted and the fetch performed on the worker's global scope (and because the ContentPolicyTypeOverridden flag is not transmitted by IPC) we will again reset tonsIContentPolicy::TYPE_FETCHand use an empty destination.
More references:
Request.contextwas introduced in https://hg.mozilla.org/mozilla-central/rev/4ac6715d8224- Around the same time, setting the context was allowed for Chrome callers in https://hg.mozilla.org/mozilla-central/rev/7fde8710208a
- Apparently this was causing issues, so in bug 1250048
OverrideContentPolicyTypestuff was introduced. There are contradicting opinions about whether or not the destination should be propagated here. Request.destinationwas introduced as a replacement to context in bug 1402892.OverrideContentPolicyTypeis used for worklet since https://hg.mozilla.org/mozilla-central/rev/367f64f2dd1b
For fetch-init.h2.html, the new Request initially creates an InternalRequest from the URL string, setting mContentPolicyType to nsIContentPolicy::TYPE_FETCH.
mozilla::dom::InternalRequest::InternalRequest(nsTSubstring<char> const&, nsTSubstring<char> const&)
mozilla::MakeSafeRefPtr<mozilla::dom::InternalRequest, nsTAutoStringN<char, 64ul>&, nsTString<char>&>(nsTAutoStringN<char, 64ul>&, nsTString<char>&)
mozilla::dom::Request::Constructor(nsIGlobalObject*, JSContext*, mozilla::dom::RequestOrUTF8String const&, mozilla::dom::RequestInit const&, mozilla::dom::CallerType, mozilla::ErrorResult&)
mozilla::dom::Request::Constructor(mozilla::dom::GlobalObject const&, mozilla::dom::RequestOrUTF8String const&, mozilla::dom::RequestInit const&, mozilla::ErrorResult&)
mozilla::dom::Request_Binding::_constructor(JSContext*, unsigned int, JS::Value*)
Just going down one line in Request::Constructor, we create a new request by calling GetRequestConstructorCopy on it, which keeps a mContentPolicyType of nsIContentPolicy::TYPE_FETCH:
mozilla::dom::InternalRequest::GetRequestConstructorCopy(nsIGlobalObject*, mozilla::ErrorResult&) const
mozilla::dom::Request::Constructor(nsIGlobalObject*, JSContext*, mozilla::dom::RequestOrUTF8String const&, mozilla::dom::RequestInit const&, mozilla::dom::CallerType, mozilla::ErrorResult&)
mozilla::dom::Request::Constructor(mozilla::dom::GlobalObject const&, mozilla::dom::RequestOrUTF8String const&, mozilla::dom::RequestInit const&, mozilla::ErrorResult&)
mozilla::dom::Request_Binding::_constructor(JSContext*, unsigned int, JS::Value*)
When the global fetch function is called, we are this time cloning the previous InternalRequest:
mozilla::dom::Request::Request::GetInternalRequest()
mozilla::dom::Request::Constructor(nsIGlobalObject*, JSContext*, mozilla::dom::RequestOrUTF8String const&, mozilla::dom::RequestInit const&, mozilla::dom::CallerType, mozilla::ErrorResult&)
mozilla::dom::Request::Constructor(mozilla::dom::GlobalObject const&, mozilla::dom::RequestOrUTF8String const&, mozilla::dom::RequestInit const&, mozilla::ErrorResult&)
mozilla::dom::FetchRequest(nsIGlobalObject*, mozilla::dom::RequestOrUTF8String const&, mozilla::dom::RequestInit const&, mozilla::dom::CallerType, mozilla::ErrorResult&)
mozilla::dom::Window_Binding::fetch(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&)
We are again calling GetRequestConstructorCopy which sets a mContentPolicyType to nsIContentPolicy::TYPE_FETCH:
mozilla::dom::InternalRequest::GetRequestConstructorCopy(nsIGlobalObject*, mozilla::ErrorResult&) const
mozilla::dom::Request::Constructor(nsIGlobalObject*, JSContext*, mozilla::dom::RequestOrUTF8String const&, mozilla::dom::RequestInit const&, mozilla::dom::CallerType, mozilla::ErrorResult&)
mozilla::dom::Request::Constructor(mozilla::dom::GlobalObject const&, mozilla::dom::RequestOrUTF8String const&, mozilla::dom::RequestInit const&, mozilla::ErrorResult&)
mozilla::dom::FetchRequest(nsIGlobalObject*, mozilla::dom::RequestOrUTF8String const&, mozilla::dom::RequestInit const&, mozilla::dom::CallerType, mozilla::ErrorResult&)
mozilla::dom::Window_Binding::fetch(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&)
Finally FetchRequest ends up creating a FetchDriver with that InternalRequest and passes it down to HttpFetch where we adjust priority in :
mozilla::dom::FetchDriver::HttpFetch(nsTSubstring<char> const&)
mozilla::dom::FetchDriver::Fetch(mozilla::dom::AbortSignalImpl*, mozilla::dom::FetchDriverObserver*)
mozilla::dom::FetchRequest(nsIGlobalObject*, mozilla::dom::RequestOrUTF8String const&, mozilla::dom::RequestInit const&, mozilla::dom::CallerType, mozilla::ErrorResult&)
mozilla::dom::Window_Binding::fetch(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&)
For the first test case of fetch-destination.https.html, setting the src attribute triggers image loading via the file that was changed in bug 1839313, which creates a channel and load info with nsIContentPolicy::TYPE_INTERNAL_IMAGE:
mozilla::net::LoadInfo::LoadInfo(nsIPrincipal*, nsIPrincipal*, nsINode*, unsigned int, nsIContentPolicy::nsContentPolicyType, mozilla::Maybe<mozilla::dom::ClientInfo> const&, mozilla::Maybe<mozilla::dom::ServiceWorkerDescriptor> const&, unsigned int, bool)
mozilla::net::nsIOService::NewChannelFromURIWithProxyFlagsInternal(nsIURI*, nsIURI*, unsigned int, nsINode*, nsIPrincipal*, nsIPrincipal*, mozilla::Maybe<mozilla::dom::ClientInfo> const&, mozilla::Maybe<mozilla::dom::ServiceWorkerDescriptor> const&, unsigned int, nsIContentPolicy::nsContentPolicyType, unsigned int, bool, nsIChannel**)
mozilla::net::nsIOService::NewChannelFromURIWithClientAndController(nsIURI*, nsINode*, nsIPrincipal*, nsIPrincipal*, mozilla::Maybe<mozilla::dom::ClientInfo> const&, mozilla::Maybe<mozilla::dom::ServiceWorkerDescriptor> const&, unsigned int, nsIContentPolicy::nsContentPolicyType, unsigned int, bool, nsIChannel**)
NS_NewChannelInternal(nsIChannel**, nsIURI*, nsINode*, nsIPrincipal*, nsIPrincipal*, mozilla::Maybe<mozilla::dom::ClientInfo> const&, mozilla::Maybe<mozilla::dom::ServiceWorkerDescriptor> const&, unsigned int, nsIContentPolicy::nsContentPolicyType, nsICookieJarSettings*, mozilla::dom::PerformanceStorage*, nsILoadGroup*, nsIInterfaceRequestor*, unsigned int, nsIIOService*, unsigned int, bool)
NS_NewChannelWithTriggeringPrincipal(nsIChannel**, nsIURI*, nsINode*, nsIPrincipal*, unsigned int, nsIContentPolicy::nsContentPolicyType, mozilla::dom::PerformanceStorage*, nsILoadGroup*, nsIInterfaceRequestor*, unsigned int, nsIIOService*)
NewImageChannel(nsIChannel**, bool*, nsIURI*, nsIURI*, mozilla::CORSMode, nsIReferrerInfo*, nsILoadGroup*, unsigned int, nsIContentPolicy::nsContentPolicyType, nsIPrincipal*, nsINode*, bool, unsigned long, mozilla::dom::FetchPriority)
imgLoader::LoadImage(nsIURI*, nsIURI*, nsIReferrerInfo*, nsIPrincipal*, unsigned long, nsILoadGroup*, imgINotificationObserver*, nsINode*, mozilla::dom::Document*, unsigned int, nsISupports*, nsIContentPolicy::nsContentPolicyType, nsTSubstring<char16_t> const&, bool, bool, unsigned long, mozilla::dom::FetchPriority, imgRequestProxy**)
nsContentUtils::LoadImage(nsIURI*, nsINode*, mozilla::dom::Document*, nsIPrincipal*, unsigned long, nsIReferrerInfo*, imgINotificationObserver*, int, nsTSubstring<char16_t> const&, imgRequestProxy*#22 0x00007f6b56bdcc36 in , nsIContentPolicy::nsContentPolicyType, bool, bool, unsigned long, mozilla::dom::FetchPriority)
nsImageLoadingContent::LoadImage(nsIURI*, bool, bool, nsImageLoadingContent::ImageLoadType, unsigned int, mozilla::dom::Document*, nsIPrincipal*)
nsImageLoadingContent::LoadImage(nsIURI, bool, bool, nsImageLoadingContent::ImageLoadType, nsIPrincipal*) (this=0x7f4ac120d108, aNewURI=0x7f4ac13cb580, aForce=true, aNotify=true, aImageLoadType=nsImageLoadingContent::eImageLoadType_Normal, aTriggeringPrincipal=0x7f4ac13b7040)
mozilla::dom::HTMLImageElement::LoadSelectedImage(bool, bool, bool) (this=0x7f4ac120d080, aForce=true, aNotify=true, aAlwaysLoad=true)
mozilla::dom::HTMLImageElement::AfterMaybeChangeAttr(int, nsAtom*, nsAttrValueOrString const&, nsAttrValue const*, nsIPrincipal*, bool)
mozilla::dom::HTMLImageElement::AfterSetAttr(int, nsAtom*, nsAttrValue const*, nsAttrValue const*, nsIPrincipal*, bool)
This is then properly copied to the ParentToParentServiceWorkerFetchEventOpArgs by ServiceWorkerPrivate::SendFetchEvent, when the channel is intercepted:
mozilla::dom::(anonymous namespace)::GetIPCInternalRequest(nsIInterceptedChannel*)
mozilla::dom::ServiceWorkerPrivate::SendFetchEvent(nsCOMPtr<nsIInterceptedChannel>, nsILoadGroup*, nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&)
mozilla::dom::(anonymous namespace)::ContinueDispatchFetchEventRunnable::Run()
mozilla::PermissionManager::WhenPermissionsAvailable(nsIPrincipal*, nsIRunnable*) (this=0x7f844de88800, aPrincipal=0x7f847945ef20, aRunnable=0x7f844e7fe940)
mozilla::dom::ServiceWorkerManager::DispatchFetchEvent(nsIInterceptedChannel*, mozilla::ErrorResult&) (this=0x7f8478bca3d0, aChannel=0x7f84690182e8, aRv=...)
mozilla::dom::ServiceWorkerInterceptController::ChannelIntercepted(nsIInterceptedChannel*) (this=0x7f846e6c5760, aChannel=0x7f84690182e8)
mozilla::net::ParentChannelListener::ChannelIntercepted(nsIInterceptedChannel*)
mozilla::net::InterceptedHttpChannel::AsyncOpenInternal()
mozilla::net::InterceptedHttpChannel::AsyncOpen(nsIStreamListener*)
mozilla::net::nsHttpChannel::OpenRedirectChannel(nsresult)
mozilla::net::nsHttpChannel::ContinueAsyncRedirectChannelToURI(nsresult)
mozilla::net::nsHttpChannel::OnRedirectVerifyCallback(nsresult)
mozilla::net::nsAsyncVerifyRedirectCallbackEvent::Run()
It then properly received by the child process to build an InternalRequest with the proper content policy:
mozilla::dom::InternalRequest::InternalRequest(mozilla::dom::IPCInternalRequest const&) (this=0x7f6b6b4cb660, aIPCRequest=...)
mozilla::MakeSafeRefPtr<mozilla::dom::InternalRequest, mozilla::dom::IPCInternalRequest const&>(mozilla::dom::IPCInternalRequest const&) (aArgs=...)
mozilla::dom::FetchEventOpProxyChild::Initialize(mozilla::dom::ParentToChildServiceWorkerFetchEventOpArgs const&) (this=0x7f6b43f518b0, aArgs=...)
mozilla::dom::RemoteWorkerChild::RecvPFetchEventOpProxyConstructor(mozilla::dom::PFetchEventOpProxyChild*, mozilla::dom::ParentToChildServiceWorkerFetchEventOpArgs const&)
mozilla::dom::PRemoteWorkerChild::OnMessageReceived(IPC::Message const&) (this=0x7f6b43f1fac0, msg__=...)
But again when the fetch method is called for the worker, GetRequestConstructorCopy sets a mContentPolicyType to nsIContentPolicy::TYPE_FETCH:
mozilla::dom::InternalRequest::GetRequestConstructorCopy(nsIGlobalObject*, mozilla::ErrorResult&) const (this=0x7f6b6b4cb660, aGlobal=0x7f6b6b430290, aRv=...)
mozilla::dom::Request::Constructor(nsIGlobalObject*, JSContext*, mozilla::dom::RequestOrUTF8String const&, mozilla::dom::RequestInit const&, mozilla::dom::CallerType, mozilla::ErrorResult&)
mozilla::dom::Request::Constructor(mozilla::dom::GlobalObject const&, mozilla::dom::RequestOrUTF8String const&, mozilla::dom::RequestInit const&, mozilla::ErrorResult&) (aGlobal=..., aInput=..., aInit=..., aRv=...)
mozilla::dom::FetchRequest(nsIGlobalObject*, mozilla::dom::RequestOrUTF8String const&, mozilla::dom::RequestInit const&, mozilla::dom::CallerType, mozilla::ErrorResult&) (aGlobal=0x7f6b6b430290, aInput=..., aInit=..., aCallerType=mozilla::dom::CallerType::NonSystem, aRv=...)
mozilla::dom::WorkerGlobalScope::Fetch(mozilla::dom::RequestOrUTF8String const&, mozilla::dom::RequestInit const&, mozilla::dom::CallerType, mozilla::ErrorResult&) (this=0x7f6b6b430200, aInput=..., aInit=..., aCallerType=mozilla::dom::CallerType::NonSystem, aRv=...)
mozilla::dom::WorkerGlobalScope_Binding::fetch(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&) (cx_=0x7f6b49039e00, obj=(JSObject * const) 0x3305a2007030 [object ServiceWorkerGlobalScope], void_self=0x7f6b6b430200, args=...)
mozilla::dom::WorkerGlobalScope_Binding::fetch_promiseWrapper(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&) (cx=0x7f6b49039e00, obj=(JSObject * const) 0x3305a2007030 [object ServiceWorkerGlobalScope], void_self=0x7f6b6b430200, args=...)
So as explained for fetch-init.h2.html, we will end up calling FetchDriver::HttpFetch with an empty destination.
For audioworklet, FetchDriver::HttpFetch a first time triggered by this backtrace:
mozilla::dom::FetchDriver::HttpFetch(nsTSubstring<char> const&)
mozilla::dom::FetchDriver::Fetch(mozilla::dom::AbortSignalImpl*, mozilla::dom::FetchDriverObserver*)
mozilla::dom::FetchRequest(nsIGlobalObject*, mozilla::dom::RequestOrUTF8String const&, mozilla::dom::RequestInit const&, mozilla::dom::CallerType, mozilla::ErrorResult&)
mozilla::dom::WorkletFetchHandler::StartFetch(JSContext*, nsIURI*, nsIURI*)
RequestDestination::Audioworklet is used because nsIContentPolicy::TYPE_INTERNAL_AUDIOWORKLET was previously set by OverrideContentPolicyType here:
mozilla::dom::InternalRequest::SetContentPolicyType(nsIContentPolicy::nsContentPolicyType) (this=0x7f2213efc660, aContentPolicyType=nsIContentPolicy::TYPE_INTERNAL_AUDIOWORKLET)
mozilla::dom::InternalRequest::OverrideContentPolicyType(nsIContentPolicy::nsContentPolicyType) (this=0x7f2213efc660, aContentPolicyType=nsIContentPolicy::TYPE_INTERNAL_AUDIOWORKLET)
mozilla::dom::Request::OverrideContentPolicyType(unsigned int) (this=0x7f22118a6700, aContentPolicyType=49)
mozilla::dom::WorkletFetchHandler::StartFetch(JSContext*, nsIURI*, nsIURI*) (this=0x7f22118a9ac0, aCx=0x7f221c539100, aURI=0x7f22175b7c10, aReferrer=0x7f22118b7d60)
mozilla::dom::StartFetchRunnable::Run() (this=0x7f22118a9e80)
Then similarly to what happens to what was explained for images, the channel is intercepted with the proper nsIContentPolicy but this one is later reset to nsIContentPolicy::TYPE_FETCH by GetRequestConstructorCopy. Indeed, the mContentPolicyTypeOverridden bit is not propagated by IPC. Finally FetchDriver::HttpFetch is called a second time with an empty destination.
|
|
Assignee | |
Comment 8•9 months ago
|
||
@smaug: Unfortunately, it seems most people who've worked on that in the past are no longer active so not sure who can help here. I'm requesting info from you since you reviewed https://hg.mozilla.org/mozilla-central/rev/367f64f2dd1b and fetchpriority patches. Do you have any opinion regarding how to proceed here? It seems weird to me that the destination is actually not used for the fetch but that's what people seem to agree about in https://bugzilla.mozilla.org/show_bug.cgi?id=1250048#c32
@valentin: Given that the destination is almost always empty for HttpFetch in our current implementation do you think it's worth spending more time on this for the initial shipping of fetchpriority?
|
||
Comment 9•9 months ago
|
||
How to proceed here how? This bug is a bit confusing.
It is not at all surprising that destination isn't used for fetch(). That destination isn't really the destination we see internally.
But sure, I guess it would make sense to pass destination around and use that information for priority too, as a hint.
Need to just ensure nothing relies on destination to be the real destination.
|
|
Assignee | |
Comment 10•9 months ago
|
||
(In reply to Olli Pettay [:smaug][bugs@pettay.fi] from comment #9)
How to proceed here how? This bug is a bit confusing.
It is not at all surprising that destination isn't used for fetch(). That destination isn't really the destination we see internally.
Right, we are essentially using nsIContentPolicy but still it is not propagated to the fetch request, which confuses me...
But sure, I guess it would make sense to pass destination around and use that information for priority too, as a hint.
Need to just ensure nothing relies on destination to be the real destination.
Checking again, it seems InterceptionContentPolicyType introduced in bug 1658869 is essentially working around the propagation issue I described in comment 7. With further propagation in InternalRequest::ToIPCInternalRequest (hopefully safe to do), I'm able to get the desired destination in the HttpFetch in the latest version of the patch. Will experiment more on that.
Thanks!
|
|
Assignee | |
Updated•9 months ago
|
|
|
Assignee | |
Comment 11•9 months ago
|
||
This interception information was introduced in bug 1658869 and is used
when calling fetch from the main thread. However, after bug 1351231 we
are not propagating that information when using PFetch, because it is
not copied by ToIPCInternalRequest here:
https://searchfox.org/mozilla-central/rev/14ff5b1650123600698f4bb8998ee02b102b8d2f/dom/fetch/Fetch.cpp#632
|
|
Assignee | |
Comment 12•9 months ago
|
||
runSingleTest() from mozilla/tests/fetch/fetchpriority/fetchpriority.js
waits for a ChildLoaded message for each test before verifying the
requests and priorities. However, the fetch-init.h2.html test sends
on ChildLoaded message for each request, which would make subsequent
tests (currently none) fail. This patch fixes that issue by waiting for
all the fetch requests to resolve/reject before sending a single message.
|
|
||
Updated•9 months ago
|
|
|
Assignee | |
Comment 13•9 months ago
|
||
This patch makes sure the RequestPriority of the internal request is
propagated to the child process when using PFetch here:
https://searchfox.org/mozilla-central/rev/14ff5b1650123600698f4bb8998ee02b102b8d2f/dom/fetch/Fetch.cpp#632
Note that IPCInternalRequest is also used in ServiceWorkerPrivate when
intercepting a request. However, propagating the RequestPriority of the
intercepted request does not seem desirable, so we just initialize with
PRIORITY_AUTO.
|
|
Assignee | |
Comment 14•9 months ago
|
||
Step 12 of [1] says that internal prirority is copied by the Request
constructor so this patch adds such a member on InternalRequest and
makes sure it is copied by InternalRequest::GetRequestConstructorCopy.
Additionally, this patch also makes sure the original request's internal
priority is preserved when calling fetch(event.request) from a worker
on an intercepted fetch event (e.g. [2] for HTML images).
[1] https://fetch.spec.whatwg.org/#dom-request
[2] https://searchfox.org/mozilla-central/rev/b3f40fd7c4671537ed29a232e76c962977650045/testing/web-platform/tests/fetch/api/request/destination/fetch-destination.https.html#37
|
|
Assignee | |
Comment 15•9 months ago
|
||
|
|
Assignee | |
Comment 16•9 months ago
|
||
I've uploaded a stack of patch that should do what we want here. I verified that HttpFetch receives the desired internal priority, destination and RequestPriority and is able to adjust the internal priority accordingly.
However, the current WPT tests relying on "http-on-opening-request" don't seem to work here. I see two calls for OnOpeningRequest, one for the original request, and one for the fetch(event.request) call from the worker when intercepting the original request. However, only the first one is notified to the test runner. And it does not seem possible to call SpecialPowers.addObserver from the worker.
Probably we would need a better approach that "inspect the browser-sent request" (see https://web-platform-tests.org/writing-tests/server-features.html).
|
|
Assignee | |
Comment 17•8 months ago
|
||
Placing some debug logs this is the output I'm getting for the image destination with fetch(event.request, {priority: high}):
[rr 21981 182054][MozDbg] [netwerk/protocol/http/HttpChannelChild.cpp:2298] mURI->GetSpecOrDefault() = "https://web-platform.test:9000/fetch/api/request/destination/resources/dummy.png?destination=image&fetchpriority=high"
[rr 21981 182093]console.error: "dummy.png?destination=image&fetchpriority=high"
[rr 21981 182123]console.error: 10
[rr 21520 182164][MozDbg] [netwerk/protocol/http/nsHttpChannel.cpp:6298] mURI->GetSpecOrDefault() = "https://web-platform.test:9000/fetch/api/request/destination/resources/dummy.png?destination=image&fetchpriority=high"
[rr 21985 182346][MozDbg] [dom/fetch/Request.cpp:426] (int)aInit.mPriority.WasPassed() = 1
[rr 21985 182349][MozDbg] [dom/fetch/Request.cpp:428] &*request = 0x7f0b844ccac0
[rr 21985 182351][MozDbg] [dom/fetch/Request.cpp:429] (int)aInit.mPriority.Value() = 0
[rr 21985 182353][MozDbg] [dom/fetch/Request.cpp:432] requestURL = "https://web-platform.test:9000/fetch/api/request/destination/resources/dummy.png?destination=image&fetchpriority=high"
[rr 21520 182430][MozDbg] [dom/fetch/FetchDriver.cpp:867] &*mRequest = 0x7fd074ece2e0
[rr 21520 182433][MozDbg] [dom/fetch/FetchDriver.cpp:870] requestURL = "https://web-platform.test:9000/fetch/api/request/destination/resources/dummy.png?destination=image&fetchpriority=high"
[rr 21520 182435][MozDbg] [dom/fetch/FetchDriver.cpp:871] (int)fetchPriority = 0
[rr 21520 182437][MozDbg] [dom/fetch/FetchDriver.cpp:872] (int)destination = 8
[rr 21520 182440][MozDbg] [netwerk/protocol/http/nsHttpChannel.cpp:6298] mURI->GetSpecOrDefault() = "https://web-platform.test:9000/fetch/api/request/destination/resources/dummy.png?destination=image&fetchpriority=high"
In process #21981, when the src attribute is set (this corresponds to what was mentioned in comment 7 for imgLoader::LoadImage), OnOpeningRequest is called and the observers notified:
0x00007fd7248ba4d8 in nsObserverList::NotifyObservers(nsISupports*, char const*, char16_t const*) (this=0x7fd73d0b9790, aSubject=0x7fd714d67788, aTopic=0x7fd71cc34a8a "http-on-opening-request", someData=0x0) at xpcom/ds/nsObserverList.cpp:71
0x00007fd7248bc2b7 in nsObserverService::NotifyObservers(nsISupports*, char const*, char16_t const*) (this=0x7fd73d0ae4c0, aSubject=0x7fd714d67788, aTopic=0x7fd71cc34a8a "http-on-opening-request", aSomeData=0x0) at xpcom/ds/nsObserverService.cpp:289
0x00007fd72542c82a in mozilla::net::nsHttpHandler::NotifyObservers(nsIChannel*, char const*) (this=0x7fd7182e4100, chan=0x7fd714d67788, event=0x7fd71cc34a8a "http-on-opening-request") at netwerk/protocol/http/nsHttpHandler.cpp:683
0x00007fd7255a50e4 in mozilla::net::nsHttpHandler::OnOpeningRequest(nsIHttpChannel*) (this=0x7fd7182e4100, chan=0x7fd714d67788) at netwerk/protocol/http/nsHttpHandler.h:357
0x00007fd72552e44f in mozilla::net::HttpChannelChild::AsyncOpenInternal(nsIStreamListener*) (this=0x7fd714d67700, aListener=0x7fd70f9f3a60) at netwerk/protocol/http/HttpChannelChild.cpp:2299
0x00007fd72552db85 in mozilla::net::HttpChannelChild::AsyncOpen(nsIStreamListener*) (this=0x7fd714d67700, aListener=0x7fd70f9f3a60) at netwerk/protocol/http/HttpChannelChild.cpp:2205
0x00007fd726cf5bd8 in imgLoader::LoadImage(nsIURI*, nsIURI*, nsIReferrerInfo*, nsIPrincipal*, unsigned long, nsILoadGroup*, imgINotificationObserver*, nsINode*, mozilla::dom::Document*, unsigned int, nsISupports*, nsIContentPolicy::nsContentPolicyType, nsTSubstring<char16_t> const&, bool, bool, unsigned long, mozilla::dom::FetchPriority, imgRequestProxy**)
(this=0x7fd71ab5d300, aURI=0x7fd70f9fe2e0, aInitialDocumentURI=0x7fd714d83970, aReferrerInfo=0x7fd70f9f60b0, aTriggeringPrincipal=0x7fd714d7cba0, aRequestContextID=0, aLoadGroup=0x7fd714dd3ac0, aObserver=0x7fd70f80e108, aContext=0x7fd70f80e080, aLoadingDocument=0x7fd71ab0f400, aLoadFlags=0, aCacheKey=0x0, aContentPolicyType=nsIContentPolicy::TYPE_INTERNAL_IMAGE, initiatorType=..., aUseUrgentStartForChannel=false, aLinkPreload=false, aEarlyHintPreloaderId=0, aFetchPriority=mozilla::dom::FetchPriority::Auto, _retval=0x7fd70f80e118)
at image/imgLoader.cpp:2514
0x00007fd726e70873 in nsContentUtils::LoadImage(nsIURI*, nsINode*, mozilla::dom::Document*, nsIPrincipal*, unsigned long, nsIReferrerInfo*, imgINotificationObserver*, int, nsTSubstring<char16_t> const&, imgRequestProxy**, nsIContentPolicy::nsContentPolicyType, bool, bool, unsigned long, mozilla::dom::FetchPriority)
(aURI=0x7fd70f9fe2e0, aContext=0x7fd70f80e080, aLoadingDocument=0x7fd71ab0f400, aLoadingPrincipal=0x7fd714d7cba0, aRequestContextID=0, aReferrerInfo=0x7fd70f9f60b0, aObserver=0x7fd70f80e108, aLoadFlags=0, initiatorType=..., aRequest=0x7fd70f80e118, aContentPolicyType=nsIContentPolicy::TYPE_INTERNAL_IMAGE, aUseUrgentStartForChannel=false, aLinkPreload=false, aEarlyHintPreloaderId=0, aFetchPriority=mozilla::dom::FetchPriority::Auto) at dom/base/nsContentUtils.cpp:4071
0x00007fd726f999a4 in nsImageLoadingContent::LoadImage(nsIURI*, bool, bool, nsImageLoadingContent::ImageLoadType, unsigned int, mozilla::dom::Document*, nsIPrincipal*) (this=0x7fd70f80e108, aNewURI=0x7fd70f9fe2e0, aForce=true, aNotify=true, aImageLoadType=nsImageLoadingContent::eImageLoadType_Normal, aLoadFlags=0, aDocument=0x7fd71ab0f400, aTriggeringPrincipal=0x7fd714d7cba0) at dom/base/nsImageLoadingContent.cpp:1144
0x00007fd7299be048 in nsImageLoadingContent::LoadImage(nsIURI*, bool, bool, nsImageLoadingContent::ImageLoadType, nsIPrincipal*) (this=0x7fd70f80e108, aNewURI=0x7fd70f9fe2e0, aForce=true, aNotify=true, aImageLoadType=nsImageLoadingContent::eImageLoadType_Normal, aTriggeringPrincipal=0x7fd714d7cba0) at dom/base/nsImageLoadingContent.h:168
0x00007fd729990c9a in mozilla::dom::HTMLImageElement::LoadSelectedImage(bool, bool, bool) (this=0x7fd70f80e080, aForce=true, aNotify=true, aAlwaysLoad=true) at dom/html/HTMLImageElement.cpp:928
0x00007fd729990132 in mozilla::dom::HTMLImageElement::AfterMaybeChangeAttr(int, nsAtom*, nsAttrValueOrString const&, nsAttrValue const*, nsIPrincipal*, bool) (this=0x7fd70f80e080, aNamespaceID=0, aName=0x7fd71ba74be0 <mozilla::detail::gGkAtoms+76056>, aValue=..., aOldValue=0x0, aMaybeScriptedPrincipal=0x7fd714d7cba0, aNotify=true) at dom/html/HTMLImageElement.cpp:467
0x00007fd72998f8d0 in mozilla::dom::HTMLImageElement::AfterSetAttr(int, nsAtom*, nsAttrValue const*, nsAttrValue const*, nsIPrincipal*, bool) (this=0x7fd70f80e080, aNameSpaceID=0, aName=0x7fd71ba74be0 <mozilla::detail::gGkAtoms+76056>, aValue=0x7ffcadd0f8f0, aOldValue=0x0, aMaybeScriptedPrincipal=0x7fd714d7cba0, aNotify=true) at dom/html/HTMLImageElement.cpp:310
I set devtools.console.stdout.content=true and the priority received by the observer is 10.
This also triggers an OnOpeningRequest on process #21520, but this does not lead to a message to the listener registered by the test:
0x00007fd0bbb3658b in mozilla::net::nsHttpChannel::AsyncOpen(nsIStreamListener*) (this=0x7fd0879d5c00, aListener=0x7fd075d4c298) at netwerk/protocol/http/nsHttpChannel.cpp:6298
0x00007fd0bb9fb775 in mozilla::net::HttpChannelParent::InvokeAsyncOpen(nsresult) (this=0x7fd085bb8700, rv=nsresult::NS_OK) at netwerk/protocol/http/HttpChannelParent.cpp:387
0x00007fd0bb9fb5d5 in mozilla::net::HttpChannelParent::TryInvokeAsyncOpen(nsresult) (this=0x7fd085bb8700, aRv=nsresult::NS_OK) at netwerk/protocol/http/HttpChannelParent.cpp:211
0x00007fd0bba36cbb in mozilla::net::HttpChannelParent::DoAsyncOpen(nsIURI*, nsIURI*, nsIURI*, nsIReferrerInfo*, nsIURI*, nsIURI*, unsigned int const&, CopyableTArray<mozilla::net::RequestHeaderTuple> const&, nsTString<char> const&, mozilla::Maybe<mozilla::ipc::IPCStream> const&, bool const&, short const&, mozilla::net::ClassOfService const&, unsigned char const&, bool const&, unsigned int const&, bool const&, unsigned long const&, nsTString<char> const&, bool const&, bool const&, bool const&, bool const&, bool const&, unsigned int const&, mozilla::net::LoadInfoArgs const&, unsigned int const&, unsigned long const&, mozilla::Maybe<mozilla::net::CorsPreflightArgs> const&, unsigned int const&, bool const&, bool const&, bool const&, nsTString<char> const&, mozilla::dom::RequestMode const&, unsigned int const&, unsigned long const&, nsTString<char16_t> const&, unsigned long const&, nsTArray<mozilla::net::PreferredAlternativeDataTypeParams> const&, unsigned long const&, mozilla::TimeStamp const&, mozilla::TimeStamp const&, mozilla::TimeStamp const&, mozilla::TimeStamp const&, mozilla::TimeStamp const&, mozilla::TimeStamp const&, bool const&, mozilla::TimeStamp const&, unsigned long const&, nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, bool const&)::$_2::operator()() const (this=0x7fd075063400) at netwerk/protocol/http/HttpChannelParent.cpp:667
This request gets intercepted as explained in comment 7 and again we end up setting the priority to -10 in FetchDriver::HttpFetch on process #21520 (with these patches taking into account the image destination). Again, this ends up calling OnOpeningRequest on process #21520, but does not lead to a message to the listener registered by the test:
mozilla::net::nsHttpChannel::SetPriority(int) (this=0x7fd0879d6600, value=-10) at netwerk/protocol/http/nsHttpChannel.cpp:6944
0x00007fd0bb9d4d41 in mozilla::net::HttpBaseChannel::AdjustPriority(int) (this=0x7fd0879d6600, delta=-20) at netwerk/protocol/http/HttpBaseChannel.cpp:4121
0x00007fd0bfb26bbd in mozilla::dom::FetchDriver::HttpFetch(nsTSubstring<char> const&) (this=0x7fd085bb8840, aPreferredAlternativeDataType=...) at dom/fetch/FetchDriver.cpp:909
0x00007fd0bfb1c8d4 in mozilla::dom::FetchDriver::Fetch(mozilla::dom::AbortSignalImpl*, mozilla::dom::FetchDriverObserver*) (this=0x7fd085bb8840, aSignalImpl=0x0, aObserver=0x7fd0ab869040) at dom/fetch/FetchDriver.cpp:485
0x00007fd0bfb349a4 in mozilla::dom::FetchService::FetchInstance::Fetch() (this=0x7fd0ab869040) at dom/fetch/FetchService.cpp:244
0x00007fd0bfb385cb in mozilla::dom::FetchService::Fetch(mozilla::Variant<mozilla::dom::FetchService::NavigationPreloadArgs, mozilla::dom::FetchService::WorkerFetchArgs, mozilla::dom::FetchService::UnknownArgs>&&) (this=0x7fd09a6d1d30, aArgs=...)
at dom/fetch/FetchService.cpp:645
0x00007fd0bfb4e763 in mozilla::dom::FetchParent::RecvFetchOp(mozilla::dom::FetchOpArgs&&)::$_2::operator()() (this=0x7fd08fe26c68) at dom/fetch/FetchParent.cpp:172
Finally, the HTTP request is performed:
mozilla::net::nsHttp::ConvertRequestHeadToString(mozilla::net::nsHttpRequestHead&, bool, bool, bool) (aRequestHead=..., aHasRequestBody=false, aRequestBodyHasHeaders=false, aUsingConnect=false) at netwerk/protocol/http/nsHttp.cpp:511
0x00007fd0bbbd5aa8 in mozilla::net::nsHttpTransaction::Init(unsigned int, mozilla::net::nsHttpConnectionInfo*, mozilla::net::nsHttpRequestHead*, nsIInputStream*, unsigned long, bool, nsIEventTarget*, nsIInterfaceRequestor*, nsITransportEventSink*, unsigned long, mozilla::net::HttpTrafficCategory, nsIRequestContext*, mozilla::net::ClassOfService, unsigned int, bool, unsigned long, std::function<void (mozilla::net::TransactionObserverResult&&)>&&, std::function<nsresult (unsigned int, nsTSubstring<char> const&, nsTSubstring<char> const&, mozilla::net::HttpTransactionShell*)>&&, mozilla::net::HttpTransactionShell*, unsigned int)
(this=0x7fd074653b00, caps=16777249, cinfo=0x7fd075047b50, requestHead=0x7fd0879d6838, requestBody=0x0, requestContentLength=0, requestBodyHasHeaders=false, target=0x7fd0d36923c0, callbacks=0x7fd09a550220, eventsink=0x7fd0879d6cf8, browserId=0, trafficCategory=mozilla::net::eN1, requestContext=0x7fd08e366b00, classOfService=..., initialRwin=0, responseTimeoutEnabled=true, channelId=46213848105405, transactionObserver=..., aOnPushCallback=..., transWithPushedStream=0x0, aPushedStreamId=0) at netwerk/protocol/http/nsHttpTransaction.cpp:268
0x00007fd0bbbd6e6e in non-virtual thunk to mozilla::net::nsHttpTransaction::Init(unsigned int, mozilla::net::nsHttpConnectionInfo*, mozilla::net::nsHttpRequestHead*, nsIInputStream*, unsigned long, bool, nsIEventTarget*, nsIInterfaceRequestor*, nsITransportEventSink*, unsigned long, mozilla::net::HttpTrafficCategory, nsIRequestContext*, mozilla::net::ClassOfService, unsigned int, bool, unsigned long, std::function<void (mozilla::net::TransactionObserverResult&&)>&&, std::function<nsresult (unsigned int, nsTSubstring<char> const&, nsTSubstring<char> const&, mozilla::net::HttpTransactionShell*)>&&, mozilla::net::HttpTransactionShell*, unsigned int) () at obj-x86_64-pc-linux-gnu-debug/dist/bin/libxul.so
0x00007fd0bbb1712c in mozilla::net::nsHttpChannel::InitTransaction() (this=0x7fd0879d6600) at netwerk/protocol/http/nsHttpChannel.cpp:1564
0x00007fd0bbb164e9 in mozilla::net::nsHttpChannel::DispatchTransaction(mozilla::net::HttpTransactionShell*) (this=0x7fd0879d6600, aTransWithStickyConn=0x0) at netwerk/protocol/http/nsHttpChannel.cpp:1070
0x00007fd0bbb14e33 in mozilla::net::nsHttpChannel::DoConnectActual(mozilla::net::HttpTransactionShell*) (this=0x7fd0879d6600, aTransWithStickyConn=0x0) at netwerk/protocol/http/nsHttpChannel.cpp:1063
0x00007fd0bbb14b17 in mozilla::net::nsHttpChannel::DoConnect(mozilla::net::HttpTransactionShell*) (this=0x7fd0879d6600, aTransWithStickyConn=0x0) at netwerk/protocol/http/nsHttpChannel.cpp:1050
0x00007fd0bbb13209 in mozilla::net::nsHttpChannel::ContinueConnect() (this=0x7fd0879d6600) at netwerk/protocol/http/nsHttpChannel.cpp:1012
0x00007fd0bbb142d7 in mozilla::net::nsHttpChannel::TriggerNetwork() (this=0x7fd0879d6600) at netwerk/protocol/http/nsHttpChannel.cpp:10039
0x00007fd0bbb2fb12 in mozilla::net::nsHttpChannel::OnCacheEntryAvailableInternal(nsICacheEntry*, bool, nsresult) (this=0x7fd0879d6600, entry=0x7fd09a550580, aNew=true, status=nsresult::NS_OK) at netwerk/protocol/http/nsHttpChannel.cpp:4448
0x00007fd0bbb2f718 in mozilla::net::nsHttpChannel::OnCacheEntryAvailable(nsICacheEntry*, bool, nsresult) (this=0x7fd0879d6600, entry=0x7fd09a550580, aNew=true, status=nsresult::NS_OK) at netwerk/protocol/http/nsHttpChannel.cpp:4378
0x00007fd0bb7ee754 in mozilla::net::CacheEntry::InvokeAvailableCallback(mozilla::net::CacheEntry::Callback const&) (this=0x7fd085bb76c0, aCallback=...) at netwerk/cache2/CacheEntry.cpp:897
0x00007fd0bb7f2c2f in mozilla::net::CacheEntry::InvokeCallback(mozilla::net::CacheEntry::Callback&) (this=0x7fd085bb76c0, aCallback=...) at netwerk/cache2/CacheEntry.cpp:811
0x00007fd0bb7f239c in mozilla::net::CacheEntry::InvokeCallbacks(bool) (this=0x7fd085bb76c0, aReadOnly=false) at netwerk/cache2/CacheEntry.cpp:674
0x00007fd0bb7ef355 in mozilla::net::CacheEntry::InvokeCallbacks() (this=0x7fd085bb76c0) at netwerk/cache2/CacheEntry.cpp:616
0x00007fd0bb7ee0ba in mozilla::net::CacheEntry::Open(mozilla::net::CacheEntry::Callback&, bool, bool, bool) (this=0x7fd085bb76c0, aCallback=..., aTruncate=false, aPriority=false, aBypassIfBusy=false) at netwerk/cache2/CacheEntry.cpp:343
0x00007fd0bb7edf2e in mozilla::net::CacheEntry::AsyncOpen(nsICacheEntryOpenCallback*, unsigned int) (this=0x7fd085bb76c0, aCallback=0x7fd0879d6cf0, aFlags=16) at netwerk/cache2/CacheEntry.cpp:318
0x00007fd0bb7e99b7 in mozilla::net::CacheStorage::AsyncOpenURI(nsIURI*, nsTSubstring<char> const&, unsigned int, nsICacheEntryOpenCallback*) (this=0x7fd09a543670, aURI=0x7fd0750b7970, aIdExtension=..., aFlags=16, aCallback=0x7fd0879d6cf0) at netwerk/cache2/CacheStorage.cpp:67
0x00007fd0bbb2b2d1 in mozilla::net::nsHttpChannel::OpenCacheEntryInternal(bool) (this=0x7fd0879d6600, isHttps=true) at netwerk/protocol/http/nsHttpChannel.cpp:3922
0x00007fd0bbb12cb3 in mozilla::net::nsHttpChannel::OpenCacheEntry(bool) (this=0x7fd0879d6600, isHttps=true) at netwerk/protocol/http/nsHttpChannel.cpp:3772
0x00007fd0bbb1257c in mozilla::net::nsHttpChannel::ConnectOnTailUnblock() (this=0x7fd0879d6600) at netwerk/protocol/http/nsHttpChannel.cpp:908
0x00007fd0bbb1187b in mozilla::net::nsHttpChannel::Connect() (this=0x7fd0879d6600) at netwerk/protocol/http/nsHttpChannel.cpp:896
Below are the relevant HTTP logs (see attached file for more details):
2024-05-27 06:04:44.656030 UTC - [Parent 21520: Main Thread]: D/nsHttp Creating HttpBaseChannel @7fd0879d5c00
2024-05-27 06:04:44.656038 UTC - [Parent 21520: Main Thread]: D/nsHttp Creating nsHttpChannel [this=7fd0879d5c00, nsIChannel=7fd0879d5c50]
2024-05-27 06:04:44.656044 UTC - [Parent 21520: Main Thread]: E/nsHttp HttpBaseChannel::Init [this=7fd0879d5c00]
2024-05-27 06:04:44.656048 UTC - [Parent 21520: Main Thread]: E/nsHttp host=web-platform.test port=9000
2024-05-27 06:04:44.656050 UTC - [Parent 21520: Main Thread]: E/nsHttp uri=https://web-platform.test:9000/fetch/api/request/destination/resources/dummy.png?destination=image&fetchpriority=high
2024-05-27 06:04:44.656076 UTC - [Parent 21520: Main Thread]: E/nsHttp nsHttpChannel::Init [this=7fd0879d5c00]
2024-05-27 06:04:44.656083 UTC - [Parent 21520: Main Thread]: D/nsHttp nsHttpChannel [this=7fd0879d5c00] SetWarningReporter [7fd085bb8750]
2024-05-27 06:04:44.656088 UTC - [Parent 21520: Main Thread]: D/nsHttp HttpBaseChannel::SetReferrerInfoInternal [this=7fd0879d5c00 aClone(0) aCompute(0)]
2024-05-27 06:04:44.656126 UTC - [Parent 21520: Main Thread]: D/nsHttp HttpBaseChannel::SetRequestHeader [this=7fd0879d5c00 header="Cookie" value="" merge=0]
2024-05-27 06:04:44.656138 UTC - [Parent 21520: Main Thread]: D/nsHttp ParentChannelListener::ParentChannelListener [this=7fd075d4c280, next=7fd085bb8740]
2024-05-27 06:04:44.656147 UTC - [Parent 21520: Main Thread]: D/nsHttp nsHttpChannel::SetCacheKey [this=7fd0879d5c00 key=0]
2024-05-27 06:04:44.656150 UTC - [Parent 21520: Main Thread]: D/nsHttp nsHttpChannel::SetAllowStaleCacheContent [this=7fd0879d5c00, allow=0]
2024-05-27 06:04:44.656155 UTC - [Parent 21520: Main Thread]: D/nsHttp nsHttpChannel::SetPriority 7fd0879d5c00 p=10
...
2024-05-27 06:04:44.660639 UTC - [Parent 21520: Main Thread]: D/nsHttp nsHttpChannel::AsyncOpen [this=7fd0879d5c00]
...
2024-05-27 06:04:44.660855 UTC - [Parent 21520: Main Thread]: D/nsHttp nsHttpHandler::NotifyObservers [this=7fd0aa868e00 chan=7fd0879d5c50 event="http-on-opening-request"]
...
2024-05-27 06:04:44.694545 UTC - [Parent 21520: Main Thread]: E/nsHttp HttpBaseChannel::Init [this=7fd0879d6600]
2024-05-27 06:04:44.694549 UTC - [Parent 21520: Main Thread]: E/nsHttp host=web-platform.test port=9000
2024-05-27 06:04:44.694550 UTC - [Parent 21520: Main Thread]: E/nsHttp uri=https://web-platform.test:9000/fetch/api/request/destination/resources/dummy.png?destination=image&fetchpriority=high
2024-05-27 06:04:44.694576 UTC - [Parent 21520: Main Thread]: E/nsHttp nsHttpChannel::Init [this=7fd0879d6600]
2024-05-27 06:04:44.694661 UTC - [Parent 21520: Main Thread]: D/nsHttp HttpBaseChannel::SetRequestHeader [this=7fd0879d6600 header="Accept" value="image/avif,image/webp,*/*" merge=0]
2024-05-27 06:04:44.694675 UTC - [Parent 21520: Main Thread]: D/nsHttp HttpBaseChannel::SetRequestHeader [this=7fd0879d6600 header="Accept-Language" value="en-US,en;q=0.5" merge=0]
2024-05-27 06:04:44.694780 UTC - [Parent 21520: Main Thread]: D/nsHttp HttpBaseChannel::SetReferrerInfoInternal [this=7fd0879d6600 aClone(0) aCompute(1)]
2024-05-27 06:04:44.695066 UTC - [Parent 21520: Main Thread]: D/nsHttp nsHttpChannel::SetPriority 7fd0879d6600 p=10
2024-05-27 06:04:44.695075 UTC - [Parent 21520: Main Thread]: D/nsHttp nsHttpChannel::SetPriority 7fd0879d6600 p=-10
2024-05-27 06:04:44.695302 UTC - [Parent 21520: Main Thread]: D/nsHttp nsHttpChannel::AsyncOpen [this=7fd0879d6600]
...
2024-05-27 06:04:44.695422 UTC - [Parent 21520: Main Thread]: D/nsHttp nsHttpHandler::NotifyObservers [this=7fd0aa868e00 chan=7fd0879d6650 event="http-on-opening-request"]
...
2024-05-27 06:04:44.707874 UTC - [Parent 21520: Main Thread]: D/nsHttp nsHttpChannel::DoConnect [this=7fd0879d6600]
2024-05-27 06:04:44.707877 UTC - [Parent 21520: Main Thread]: D/nsHttp nsHttpChannel::DoConnectActual [this=7fd0879d6600, aTransWithStickyConn=0]
2024-05-27 06:04:44.707879 UTC - [Parent 21520: Main Thread]: D/nsHttp nsHttpChannel::SetupChannelForTransaction [this=7fd0879d6600, cos=0, inc=0 prio=-10]
2024-05-27 06:04:44.707885 UTC - [Parent 21520: Main Thread]: D/nsHttp HttpBaseChannel::SetRequestHeader [this=7fd0879d6600 header="Priority" value="u=4" merge=0]
2024-05-27 06:04:44.707906 UTC - [Parent 21520: Main Thread]: D/nsHttp nsHttpChannel::DispatchTransaction [this=7fd0879d6600, aTransWithStickyConn=0]
2024-05-27 06:04:44.707917 UTC - [Parent 21520: Main Thread]: D/nsHttp Creating nsHttpTransaction @7fd074653b00
2024-05-27 06:04:44.707921 UTC - [Parent 21520: Main Thread]: E/nsHttp nsHttpChannel 7fd0879d6600 created nsHttpTransaction 7fd074653b10
2024-05-27 06:04:44.707942 UTC - [Parent 21520: Main Thread]: E/nsHttp nsHttpTransaction::Init [this=7fd074653b00 caps=1000021]
2024-05-27 06:04:44.707945 UTC - [Parent 21520: Main Thread]: E/nsHttp nsHttpTransaction 7fd074653b00 SetRequestContext 7fd08e366b00
2024-05-27 06:04:44.707971 UTC - [Parent 21520: Main Thread]: E/nsHttp http request [
2024-05-27 06:04:44.707975 UTC - [Parent 21520: Main Thread]: E/nsHttp GET /fetch/api/request/destination/resources/dummy.png?destination=image&fetchpriority=high HTTP/1.1
2024-05-27 06:04:44.707978 UTC - [Parent 21520: Main Thread]: E/nsHttp Host: web-platform.test:9000
2024-05-27 06:04:44.707981 UTC - [Parent 21520: Main Thread]: E/nsHttp User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:127.0) Gecko/20100101 Firefox/127.0
2024-05-27 06:04:44.707983 UTC - [Parent 21520: Main Thread]: E/nsHttp Accept: image/avif,image/webp,*/*
2024-05-27 06:04:44.707991 UTC - [Parent 21520: Main Thread]: E/nsHttp Accept-Language: en-US,en;q=0.5
2024-05-27 06:04:44.707993 UTC - [Parent 21520: Main Thread]: E/nsHttp Accept-Encoding: gzip, deflate, br, zstd
2024-05-27 06:04:44.707996 UTC - [Parent 21520: Main Thread]: E/nsHttp Referer: https://web-platform.test:9000/_mozilla/fetch/fetchpriority/support/fetch-tests/resources/fetch-destination-worker.js
2024-05-27 06:04:44.707999 UTC - [Parent 21520: Main Thread]: E/nsHttp Connection: keep-alive
2024-05-27 06:04:44.708001 UTC - [Parent 21520: Main Thread]: E/nsHttp Sec-Fetch-Dest: empty
2024-05-27 06:04:44.708002 UTC - [Parent 21520: Main Thread]: E/nsHttp Sec-Fetch-Mode: no-cors
2024-05-27 06:04:44.708004 UTC - [Parent 21520: Main Thread]: E/nsHttp Sec-Fetch-Site: same-origin
2024-05-27 06:04:44.708005 UTC - [Parent 21520: Main Thread]: E/nsHttp Priority: u=4
2024-05-27 06:04:44.708007 UTC - [Parent 21520: Main Thread]: E/nsHttp
2024-05-27 06:04:44.708008 UTC - [Parent 21520: Main Thread]: E/nsHttp ]
|
|
Assignee | |
Comment 18•8 months ago
|
||
@valentin: I meant to debug this a bit more but was distracted by other tasks. Anyway, this is done now and the TL;DR from comment 16 and comment 17 is that the current C++ changes from these patches look correct to me (we set the desired priority before the actual HTTP request is sent) but due to all these multi-process stuff we actually don't notify the observer for http-on-opening-request registered in the WPT test. A similar issue was discussed in bug 806753 and it seems people agree it was not worth bothering with IPC messages ensure observers are notified. Also, I'm not really sure if/where the internal priority is used when building the HTTP request and so whether we could just have tests that just check these requests on the server side, as I previously commented. So do you think we could go ahead with these patches, relying only on my manual testing for now?
|
||
Comment 19•8 months ago
|
||
Thank you, Frédéric. I had a quick look at your patches and I think it's the direction we want to go in. Feel free to request review when ready.
The fetchpriority will be used by the connection as soon as I land bug 1864392 - hopefully today or tomorrow.
|
|
Assignee | |
Comment 20•8 months ago
|
||
(In reply to Valentin Gosu [:valentin] (he/him) from comment #19)
Thank you, Frédéric. I had a quick look at your patches and I think it's the direction we want to go in. Feel free to request review when ready.
The fetchpriority will be used by the connection as soon as I land bug 1864392 - hopefully today or tomorrow.
Thanks I saw this bug but the title refers to HTTP/3... IIUC, urgency will be adjusted for HTTP/2 too. It seems we could write a test checking the HTTP header after bug 1864392, but I guess I can already send the patches (except the broken D210286) for review now.
|
|
Assignee | |
Updated•8 months ago
|
|
|
Assignee | |
Updated•8 months ago
|
|
|
||
Updated•8 months ago
|
|
|
||
Updated•8 months ago
|
|
|
||
Updated•8 months ago
|
|
|
||
Updated•8 months ago
|
|
|
||
Updated•8 months ago
|
|
|
Assignee | |
Comment 21•8 months ago
•
|
||
There are still some issues I need to figure out, essentially:
-
I'm not covering all the cases from D208467. Actually I'm not sure how to reach some stuff like xslt, worker or track...
-
My patches forward the (default) internal priority from the first request and then adjust that priority again in FetchDriver.cpp. That's correct for images, but not in other cases (e.g. using link_preload_script adjustments on top of a script loaded from the head gives wrong expectations).
-
The image tests seem to be flaky for some reason.
|
|
||
Updated•8 months ago
|
|
|
||
Updated•8 months ago
|
|
|
||
Updated•8 months ago
|
|
|
||
Updated•8 months ago
|
|
|
||
Updated•8 months ago
|
|
|
||
Updated•8 months ago
|
|
|
||
Updated•8 months ago
|
|
||
Comment 22•8 months ago
|
||
|
||
Comment 23•8 months ago
|
||
Backed out for causing base-toolchains build bustages on FetchDriver.cpp.
[task 2024-06-11T16:31:14.859Z] 16:31:14 INFO - gmake[4]: Entering directory '/builds/worker/workspace/obj-build/dom/fetch'
[task 2024-06-11T16:31:14.862Z] 16:31:14 INFO - /builds/worker/fetches/sccache/sccache /builds/worker/fetches/gcc/bin/g++ --sysroot /builds/worker/fetches/sysroot-x86_64-linux-gnu -std=gnu++17 -isystem /builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/include/c++/8 -isystem /builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/include/x86_64-linux-gnu/c++/8 -isystem /builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/include/x86_64-linux-gnu -isystem /builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/include -o Unified_cpp_dom_fetch0.o -c -I/builds/worker/workspace/obj-build/dist/stl_wrappers -I/builds/worker/workspace/obj-build/dist/system_wrappers -include /builds/worker/checkouts/gecko/config/gcc_hidden.h -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -DDEBUG=1 -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -DSTATIC_EXPORTABLE_JS_API -I/builds/worker/checkouts/gecko/dom/fetch -I/builds/worker/workspace/obj-build/dom/fetch -I/builds/worker/checkouts/gecko/dom/base -I/builds/worker/checkouts/gecko/netwerk/base -I/builds/worker/checkouts/gecko/netwerk/protocol/data -I/builds/worker/checkouts/gecko/netwerk/protocol/http -I/builds/worker/workspace/obj-build/ipc/ipdl/_ipdlheaders -I/builds/worker/checkouts/gecko/ipc/chromium/src -I/builds/worker/workspace/obj-build/dist/include -I/builds/worker/workspace/obj-build/dist/include/nspr -I/builds/worker/workspace/obj-build/dist/include/nss -DMOZILLA_CLIENT -include /builds/worker/workspace/obj-build/mozilla-config.h -D_GLIBCXX_USE_CXX11_ABI=0 -fno-rtti -pthread -fno-sized-deallocation -fno-aligned-new -ffunction-sections -fdata-sections -fno-math-errno -fno-exceptions -fno-exceptions -fPIC -gdwarf-4 -O2 -fno-omit-frame-pointer -funwind-tables -Werror -Wall -Wempty-body -Wignored-qualifiers -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wno-invalid-offsetof -Wduplicated-cond -Wimplicit-fallthrough -Wlogical-op -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=free-nonheap-object -Wno-multistatement-macros -Wno-error=class-memaccess -Wformat -Wformat-overflow=2 -Wno-psabi -Wno-error=builtin-macro-redefined -fno-strict-aliasing -ffp-contract=off -MD -MP -MF .deps/Unified_cpp_dom_fetch0.o.pp Unified_cpp_dom_fetch0.cpp
[task 2024-06-11T16:31:14.863Z] 16:31:14 INFO - In file included from /builds/worker/workspace/obj-build/dist/include/nsTObserverArray.h:12,
[task 2024-06-11T16:31:14.863Z] 16:31:14 INFO - from /builds/worker/workspace/obj-build/dist/include/mozilla/dom/AbortFollower.h:12,
[task 2024-06-11T16:31:14.863Z] 16:31:14 INFO - from /builds/worker/workspace/obj-build/dist/include/mozilla/dom/AbortSignal.h:11,
[task 2024-06-11T16:31:14.864Z] 16:31:14 INFO - from /builds/worker/workspace/obj-build/dist/include/mozilla/dom/BodyConsumer.h:10,
[task 2024-06-11T16:31:14.864Z] 16:31:14 INFO - from /builds/worker/workspace/obj-build/dist/include/mozilla/dom/Blob.h:10,
[task 2024-06-11T16:31:14.865Z] 16:31:14 INFO - from /builds/worker/workspace/obj-build/dist/include/mozilla/dom/File.h:10,
[task 2024-06-11T16:31:14.865Z] 16:31:14 INFO - from /builds/worker/checkouts/gecko/dom/fetch/BodyExtractor.cpp:8,
[task 2024-06-11T16:31:14.865Z] 16:31:14 INFO - from Unified_cpp_dom_fetch0.cpp:2:
[task 2024-06-11T16:31:14.865Z] 16:31:14 INFO - /builds/worker/workspace/obj-build/dist/include/nsTArray.h: In instantiation of 'static void AssignRangeAlgorithm<true, true>::implementation(ElemType*, IndexType, SizeType, const Item*) [with Item = mozilla::dom::workerinternals::JSSettings::JSGCSetting; ElemType = mozilla::dom::workerinternals::JSSettings::JSGCSetting; IndexType = long unsigned int; SizeType = long unsigned int]':
[task 2024-06-11T16:31:14.866Z] 16:31:14 INFO - /builds/worker/workspace/obj-build/dist/include/nsTArray.h:2439:58: required from 'void nsTArray_Impl<E, Alloc>::AssignRange(nsTArray_Impl<E, Alloc>::index_type, nsTArray_Impl<E, Alloc>::size_type, const Item*) [with Item = mozilla::dom::workerinternals::JSSettings::JSGCSetting; E = mozilla::dom::workerinternals::JSSettings::JSGCSetting; Alloc = nsTArrayInfallibleAllocator; nsTArray_Impl<E, Alloc>::index_type = long unsigned int; nsTArray_Impl<E, Alloc>::size_type = long unsigned int]'
[task 2024-06-11T16:31:14.867Z] 16:31:14 INFO - /builds/worker/workspace/obj-build/dist/include/nsTArray.h:2468:5: required from 'typename ActualAlloc::ResultType nsTArray_Impl<E, Alloc>::AssignInternal(const Item*, nsTArray_Impl<E, Alloc>::size_type) [with ActualAlloc = nsTArrayInfallibleAllocator; Item = mozilla::dom::workerinternals::JSSettings::JSGCSetting; E = mozilla::dom::workerinternals::JSSettings::JSGCSetting; Alloc = nsTArrayInfallibleAllocator; typename ActualAlloc::ResultType = void; nsTArray_Impl<E, Alloc>::size_type = long unsigned int]'
[task 2024-06-11T16:31:14.868Z] 16:31:14 INFO - /builds/worker/workspace/obj-build/dist/include/nsTArray.h:1454:39: required from 'typename ActualAlloc::ResultType nsTArray_Impl<E, Alloc>::Assign(const nsTArray_Impl<E, Allocator>&) [with Allocator = nsTArrayInfallibleAllocator; ActualAlloc = nsTArrayInfallibleAllocator; E = mozilla::dom::workerinternals::JSSettings::JSGCSetting; Alloc = nsTArrayInfallibleAllocator; typename ActualAlloc::ResultType = void]'
[task 2024-06-11T16:31:14.868Z] 16:31:14 INFO - /builds/worker/workspace/obj-build/dist/include/nsTArray.h:2971:7: required from 'CopyableTArray<E>& CopyableTArray<E>::operator=(const CopyableTArray<E>&) [with E = mozilla::dom::workerinternals::JSSettings::JSGCSetting]'
[task 2024-06-11T16:31:14.870Z] 16:31:14 INFO - /builds/worker/workspace/obj-build/dist/include/mozilla/dom/workerinternals/JSSettings.h:24:8: required from here
[task 2024-06-11T16:31:14.871Z] 16:31:14 WARNING - /builds/worker/workspace/obj-build/dist/include/nsTArray.h:671:13: warning: 'void* memcpy(void*, const void*, size_t)' writing to an object of non-trivially copyable type 'struct mozilla::dom::workerinternals::JSSettings::JSGCSetting'; use copy-assignment or copy-initialization instead [-Wclass-memaccess]
[task 2024-06-11T16:31:14.871Z] 16:31:14 INFO - memcpy(aElements + aStart, aValues, aCount * sizeof(ElemType));
[task 2024-06-11T16:31:14.872Z] 16:31:14 INFO - ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[task 2024-06-11T16:31:14.872Z] 16:31:14 INFO - In file included from /builds/worker/workspace/obj-build/dist/include/mozilla/dom/workerinternals/RuntimeService.h:18,
[task 2024-06-11T16:31:14.873Z] 16:31:14 INFO - from /builds/worker/checkouts/gecko/dom/fetch/Fetch.h:24,
[task 2024-06-11T16:31:14.873Z] 16:31:14 INFO - from /builds/worker/checkouts/gecko/dom/fetch/Fetch.cpp:7,
[task 2024-06-11T16:31:14.873Z] 16:31:14 INFO - from Unified_cpp_dom_fetch0.cpp:20:
[task 2024-06-11T16:31:14.874Z] 16:31:14 INFO - /builds/worker/workspace/obj-build/dist/include/mozilla/dom/workerinternals/JSSettings.h:25:10: note: 'struct mozilla::dom::workerinternals::JSSettings::JSGCSetting' declared here
[task 2024-06-11T16:31:14.874Z] 16:31:14 INFO - struct JSGCSetting {
[task 2024-06-11T16:31:14.874Z] 16:31:14 INFO - ^~~~~~~~~~~
[task 2024-06-11T16:31:14.874Z] 16:31:14 INFO - In file included from Unified_cpp_dom_fetch0.cpp:38:
[task 2024-06-11T16:31:14.875Z] 16:31:14 INFO - /builds/worker/checkouts/gecko/dom/fetch/FetchDriver.cpp: In lambda function:
[task 2024-06-11T16:31:14.875Z] 16:31:14 ERROR - /builds/worker/checkouts/gecko/dom/fetch/FetchDriver.cpp:898:7: error: control reaches end of non-void function [-Werror=return-type]
[task 2024-06-11T16:31:14.876Z] 16:31:14 INFO - }();
[task 2024-06-11T16:31:14.880Z] 16:31:14 INFO - ^
[task 2024-06-11T16:31:14.880Z] 16:31:14 INFO - cc1plus: all warnings being treated as errors
[task 2024-06-11T16:31:14.881Z] 16:31:14 ERROR - gmake[4]: *** [/builds/worker/checkouts/gecko/config/rules.mk:690: Unified_cpp_dom_fetch0.o] Error 1
[task 2024-06-11T16:31:14.881Z] 16:31:14 INFO - gmake[4]: Leaving directory '/builds/worker/workspace/obj-build/dom/fetch'
|
|
Assignee | |
Comment 24•8 months ago
|
||
It seems some compiler think some unreachable code is reachable, will add a MOZ_ASSERT_UNREACHABLE
|
|
||
Comment 25•8 months ago
|
||
|
||
Comment 26•8 months ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/7d3e64562102
https://hg.mozilla.org/mozilla-central/rev/32f68cf9630d
https://hg.mozilla.org/mozilla-central/rev/88e4e167739b
https://hg.mozilla.org/mozilla-central/rev/d9e55e62e34a
https://hg.mozilla.org/mozilla-central/rev/b2277ba2cbdf
https://hg.mozilla.org/mozilla-central/rev/a8dca6a560dc
Description
•