Make rel=preload work with no-cache resources.

I have a working version for this with one problem. Description of the architecture: I make use of our mem-only cache. I continue using normal httpChannel to make use of all existing security checks. it is possible to eliminate use of httpChannel as the next step. Caching will be done on the parent process to make use of our standard httpChannel for fetching. (It could be done on the child process, but we will need to implement divertToParent without HttpChannleParent). What it does: uriloader/prefetch/nsIPreloadServiceBackend.idl uriloader/prefetch/nsPreloadServiceBackend.cpp nsPreloadServiceBackend is the backend that keeps mem-only cache entry alive. There can be only one preload resource for a windowId and uri. A mem-only cache entry is alive until a real load for that resource takes it, until its link rel=preload is removed or an navigation leaves the main doc (or tab closing). nsPreloadServiceBackend is control by nsPrefetchService that are on the child process. Redirects: if a rel=preload resource response is a redirect nsPreloadServiceBackend will follow the redirects and save the last resource to the mem-only cache. Failed respose are not stored, e.g. 404 will not be in the preloaded mem-only cache. nsPrefetchService.cpp: 1) I split the preload and prefetch node lists. 2) nsPrefetchService opens a preload http channel. this is the same as until now. It also sets nsIHttpChannelInternal parameter preload to true and preloadWindowId the id of the window for which this resource is valid. The nsHttpChannel(this is on the parent process) will contact nsPreloadServiceBackend and add a new preloadNodeBackend and fills its mem-only cache entry. 3) nsPrefetchService is also responsible to cancel and delete preloaded resources, i.e. calls nsPreloadServiceBackend::RemovePreload that will release the mem-only cache entry. The resource is deleted if <link> is removed or a user navigates away from the resource (nsPrefetchService listent to DOM_WINDOW_DESTROYED_TOPIC). nsHttpChannel.cpp: for non preload request, httpChannel will query nsPreloadServiceBackend to find if there is a preloaded resource for this request. if it exist httpChannel will take data from this preloaded mem-only cache entry.

I have one problem: how to find windowId from nsINode. For link headers it works just fine but for <link rel=preloads triggered by ./dom/base/Link.cpp does not work properly. The code that looks for winId is in nsPrefetchService::Preload I want to use innerWindowId so that a refresh will remove rel=preloads. I hope I understood the meaning of innerWindowId correctly. The example of test that is failing is dom/base/test/test_preload_redirect.html (this is a new test) Olli, can you help me with windowId problem?

Comment on attachment 8902234 [details] [diff] [review] bug_preload_nocache_v1.patch This patch is almost ready. Probably missing some comments, but there is a description in the bug. Can you take a look?

(In reply to Dragana Damjanovic [:dragana] from comment #3) > Can you take a look? Yep, early tomorrow with fresh eyes.

(In reply to Dragana Damjanovic [:dragana] from comment #2) > Created attachment 8902234 [details] [diff] [review] > bug_preload_nocache_v1.patch > > I have one problem: how to find windowId from nsINode. For link headers it > works just fine but for <link rel=preloads triggered by ./dom/base/Link.cpp > does not work properly. link->GetElement()->OwnerDoc()->InnerWindowID()

(In reply to Olli Pettay [:smaug] from comment #5) > (In reply to Dragana Damjanovic [:dragana] from comment #2) > > Created attachment 8902234 [details] [diff] [review] > > bug_preload_nocache_v1.patch > > > > I have one problem: how to find windowId from nsINode. For link headers it > > works just fine but for <link rel=preloads triggered by ./dom/base/Link.cpp > > does not work properly. > > link->GetElement()->OwnerDoc()->InnerWindowID() This gives the same result as my code. The test code is simple: adds link rel=preload as=image, when preloaded is done adds a img: var link = document.createElement("LINK"); link.setAttribute("rel", "preload"); link.setAttribute("href", "http://mochi.test:8888/tests/dom/base/test/square_redirect.png"); link.setAttribute("as", "image"); link.addEventListener("load", () => { var img = document.createElement("img"); img.src = "http://mochi.test:8888/tests/dom/base/test/square_redirect.png"; document.head.appendChild(img); }); document.head.appendChild(link); The value of link->GetElement()->OwnerDoc()->InnerWindowID() is 0x80000008 (the outer windowId is0x80000005). At the same time the httpChannel created for this preload has winIds: inner=80000004 outer=80000001 (the channel has principal of the link node and also its loadGroup). Do you know what am I doing wrong here? And the channel created for <image> has as well winIds: inner=80000004 outer=80000001

Are you taking windowIDs from wrong channel? Or is wrong id passed to the channel? I'm not familiar with that part of necko, so would need to read the code to see which id is passed there. Is it for example the top level content window id, when link->GetElement()->OwnerDoc()->InnerWindowID() deals with any window, also iframes.

httpChannelInternal->SetPreloadWindowId(mTopWinId); for example looks weird. Are we dealing with top level window IDs or any window IDs?

(In reply to Olli Pettay [:smaug] from comment #8) > httpChannelInternal->SetPreloadWindowId(mTopWinId); for example looks weird. > Are we dealing with top level window IDs or any window IDs? Forget about this for now, i have added it only to because I had the mismatch described above. Ideally I should remove this.

(In reply to Olli Pettay [:smaug] from comment #7) > Are you taking windowIDs from wrong channel? Or is wrong id passed to the > channel? > I'm not familiar with that part of necko, so would need to read the code to > see which id is passed there. > Is it for example the top level content window id, when > link->GetElement()->OwnerDoc()->InnerWindowID() deals with any window, also > iframes. HttpChannel gets its windowId in this code: https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelChild.cpp#2494 HttpChannel gets content (inner)WindowId and the outerWindowId. Ideally I would like to get the same from a Link. And I will scope preloads on the innerWindowId.

From the preload drft: "Conceptually, a preloaded response ought to be committed to the HTTP cache, as it is initiated by the client, and also be available in the memory cache and be re-usable at least once within the lifetime of a fetch group." The preload resource should be available during the lifetime of a fetch group.

(In reply to Dragana Damjanovic [:dragana] from comment #10) > https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/ > HttpChannelChild.cpp#2494 > > HttpChannel gets content (inner)WindowId and the outerWindowId. That is the top level window then. Perhaps then access link->element->OwnerDoc()->GetDocShell()->GetTabChild() and do the same thing what HttpChannelChild does with it. Note, all that is of course e10s-only. Perhaps we don't need to support this stuff in non-e10s mode?

Thanks Olli. Reading the draft again, it does not say much about scoping of the preload resource. I will scope it by a fetch group, so that preloads from a iframe are available only in that iframe not the tab. So link->GetElement->OwnerDoc()->InnerWindowID() is the right id.

Comment on attachment 8902234 [details] [diff] [review] bug_preload_nocache_v1.patch Review of attachment 8902234 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/protocol/http/nsHttpChannel.cpp @@ +557,5 @@ > + MOZ_ASSERT(mPreloadWindowId); > + nsCOMPtr<nsIPreloadServiceBackend> preloadServiceBackend(do_GetService(NS_PRELOADSERVICEBACKEND_CONTRACTID)); > + LOG(("nsHttpChannel::Connect preloadServiceBackend=%p.", preloadServiceBackend.get())); > + if (preloadServiceBackend) { > + if (NS_FAILED(preloadServiceBackend->InitPreload(this, so, this is nsHttpChannel that is responsible for performing the preload, right? (mPreload == true) InitPreload(this) calls nsPreloadNodeBackend::OpenCacheEntry which suspends the channel. resume happens when the preload is canceled or when the entry is obtained. but when there is a valid cache entry already in the regular cache, we load from that cache entry? if the entry is fresh enough (so that no revalidation is needed) or later revalidated with 304, do we load that entry and copy it to the mem-only preload isolated entry and then back to the regular when the real load finds it? Or how is that done? @@ +3903,5 @@ > mRequestHead.HasHeader(nsHttp::If_Unmodified_Since) || > mRequestHead.HasHeader(nsHttp::If_Match) || > mRequestHead.HasHeader(nsHttp::If_Range); > > + if (!mPreload) { probably undesired for posts, and probably some other conditions what about when the load has to go from appcache? (has to choose - top level, or is assigned an appcache?) @@ +3904,5 @@ > mRequestHead.HasHeader(nsHttp::If_Match) || > mRequestHead.HasHeader(nsHttp::If_Range); > > + if (!mPreload) { > + nsCOMPtr<nsIPreloadServiceBackend> preloadServiceBackend(do_GetService(NS_PRELOADSERVICEBACKEND_CONTRACTID)); (would be nice to cache this, maybe on handler?) @@ +3921,5 @@ > + NS_ENSURE_SUCCESS(rv, rv); > + > + if ((mClassOfService & nsIClassOfService::Leader) || > + (mLoadFlags & LOAD_INITIAL_DOCUMENT_URI)) > + cacheEntryOpenFlags |= nsICacheStorage::OPEN_PRIORITY; OPEN_PRIORITY has no effect on mem storage @@ +7609,5 @@ > + if (mPreloadNode) { > + if (mPreload) { > + mPreloadNode->ChannelDone(status); > + } else { > + mPreloadNode->Done(); some better naming is probably needed ::: uriloader/prefetch/nsIPreloadServiceBackend.idl @@ +31,5 @@ > + out nsIStreamListener aOut); > + > + void channelDone(in nsresult aStatus); > + > + void done(); i definitely lack comments... ::: uriloader/prefetch/nsPreloadServiceBackend.cpp @@ +99,5 @@ > + > + LOG(("nsPreloadNodeBackend::AttachPreloadCache OpenOutputStream " > + "[this=%p state=%d cache=%p]", > + this, mState, mCacheEntry.get())); > + rv = mCacheEntry->OpenOutputStream(0, getter_AddRefs(out)); note that if you have not called metaDataReady on this entry before, this (opening the output stream) will release pending openers so that they will get the OnCacheEntryCheck/Available callbacks immediately. I don't say it's wrong, just FYI. and note that there can be only one output stream opened on an entry at one time. @@ +126,5 @@ > + return NS_OK; > + > +cache_error: > + MOZ_ASSERT(mCacheEntry); > + mCacheEntry->ForceValidFor(PRELOAD_FORCE_VALID_DEFAULT); (0) ? @@ +234,5 @@ > + mState = PRELOAD_DO_NOT_STORE; > + return rv; > + } > + nsAutoCString extension; > + extension.Append(nsPrintfCString("preload=u%" PRIu64, mTopWinId)); (I think you can use nsPrintfCString directly, no need to copy) @@ +259,5 @@ > + mHttpChannel->Suspend(); > + > + rv = cacheStorage->AsyncOpenURI(uri, extension, > + nsICacheStorage::OPEN_NORMALLY | > + nsICacheStorage::CHECK_MULTITHREADED, this); as I understand, there is always only one preload per winid+uri, right? hence, there will never be an entry found - you will always get a new one, right? if so, you can use OpenTruncate and get the entry right away. it will be empty, you can then set metadata on it and write to its output stream the same way as you do when obtained with OnCacheEntryAvailable. @@ +312,5 @@ > + if (mState == PRELOAD_DO_NOT_STORE){ > + return NS_ERROR_ABORT; > + } > + > + mHttpChannel->Resume(); calling resume is super-sensitive. i'd rather see a flag "resume call needed" that when true, you will call resume() where you want and then drop that flag. dtor should assert "resume call needed == false" then. @@ +385,5 @@ > + nsCOMPtr<nsIHttpChannelInternal> httpChannelInternal = > + do_QueryInterface(mRedirectChannel); > + if (httpChannelInternal) { > + httpChannelInternal->SetPreload(true); > + httpChannelInternal->SetPreloadWindowId(mTopWinId); will this preload get canceled when the link is removed? is the original channel preload mem cache entry made available (set "having metadata") to real channels? what about a redirect to the same uri? something we allow and also is sometimes common: redirect to self sets different cookies, sometimes even in a loop ; the redirect to self even more suggests using OpenTrancate to get the mem-cache preload entry, OpenTrance also always dooms any existing entry to create a new one @@ +570,5 @@ > + > + if (!strcmp(aTopic, NS_XPCOM_SHUTDOWN_OBSERVER_ID)) { > + RemoveAll(); > + mShutdown = true; > + } else if (!strcmp(aTopic, NS_PREFBRANCH_PREFCHANGE_TOPIC_ID)) { don't like Preferences::AddBoolVarCache ? ;) it's actually preferred since the preferences getters (GetBool) are doing a hash lookup that is expensive ::: uriloader/prefetch/nsPreloadServiceBackend.h @@ +74,5 @@ > + bool Take(); > + > + RefPtr<nsPreloadServiceBackend> mService; > + nsCOMPtr<nsIHttpChannel> mHttpChannel; > + nsCOMPtr<nsIHttpChannel> mRedirectChannel; ok, preloads are IIUC allowed only for http, so this is OK. note that we generally allow http redirs to different schemes.

(In reply to Honza Bambas (:mayhemer) from comment #14) > Comment on attachment 8902234 [details] [diff] [review] > bug_preload_nocache_v1.patch > > Review of attachment 8902234 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: netwerk/protocol/http/nsHttpChannel.cpp > @@ +557,5 @@ > > + MOZ_ASSERT(mPreloadWindowId); > > + nsCOMPtr<nsIPreloadServiceBackend> preloadServiceBackend(do_GetService(NS_PRELOADSERVICEBACKEND_CONTRACTID)); > > + LOG(("nsHttpChannel::Connect preloadServiceBackend=%p.", preloadServiceBackend.get())); > > + if (preloadServiceBackend) { > > + if (NS_FAILED(preloadServiceBackend->InitPreload(this, > > so, this is nsHttpChannel that is responsible for performing the preload, > right? (mPreload == true) InitPreload(this) calls > nsPreloadNodeBackend::OpenCacheEntry which suspends the channel. resume > happens when the preload is canceled or when the entry is obtained. > > but when there is a valid cache entry already in the regular cache, we load > from that cache entry? if the entry is fresh enough (so that no > revalidation is needed) or later revalidated with 304, do we load that entry > and copy it to the mem-only preload isolated entry and then back to the > regular when the real load finds it? Or how is that done? > We will copy the entry to mem-only entry. The real load will not copy it back to cache it is only going to read from the mem-only entry. > @@ +3903,5 @@ > > mRequestHead.HasHeader(nsHttp::If_Unmodified_Since) || > > mRequestHead.HasHeader(nsHttp::If_Match) || > > mRequestHead.HasHeader(nsHttp::If_Range); > > > > + if (!mPreload) { > > probably undesired for posts, and probably some other conditions > what about when the load has to go from appcache? (has to choose - top > level, or is assigned an appcache?) > I thought I did that, but I will check again (maybe I only did it for if(mPreload)) > @@ +3904,5 @@ > > mRequestHead.HasHeader(nsHttp::If_Match) || > > mRequestHead.HasHeader(nsHttp::If_Range); > > > > + if (!mPreload) { > > + nsCOMPtr<nsIPreloadServiceBackend> preloadServiceBackend(do_GetService(NS_PRELOADSERVICEBACKEND_CONTRACTID)); > > (would be nice to cache this, maybe on handler?) > ok > @@ +3921,5 @@ > > + NS_ENSURE_SUCCESS(rv, rv); > > + > > + if ((mClassOfService & nsIClassOfService::Leader) || > > + (mLoadFlags & LOAD_INITIAL_DOCUMENT_URI)) > > + cacheEntryOpenFlags |= nsICacheStorage::OPEN_PRIORITY; > > OPEN_PRIORITY has no effect on mem storage I will remove it, I was just copying everything to be sure it is correct. > > @@ +7609,5 @@ > > + if (mPreloadNode) { > > + if (mPreload) { > > + mPreloadNode->ChannelDone(status); > > + } else { > > + mPreloadNode->Done(); > > some better naming is probably needed > yes. > ::: uriloader/prefetch/nsIPreloadServiceBackend.idl > @@ +31,5 @@ > > + out nsIStreamListener aOut); > > + > > + void channelDone(in nsresult aStatus); > > + > > + void done(); > > i definitely lack comments... > I feel a bit bad letting you review this without comments. > ::: uriloader/prefetch/nsPreloadServiceBackend.cpp > @@ +99,5 @@ > > + > > + LOG(("nsPreloadNodeBackend::AttachPreloadCache OpenOutputStream " > > + "[this=%p state=%d cache=%p]", > > + this, mState, mCacheEntry.get())); > > + rv = mCacheEntry->OpenOutputStream(0, getter_AddRefs(out)); > > note that if you have not called metaDataReady on this entry before, this > (opening the output stream) will release pending openers so that they will > get the OnCacheEntryCheck/Available callbacks immediately. I don't say it's > wrong, just FYI. > This trying/failing untill I got it right, I think. I call DoAddCacheEntryHeaders and mCacheEntry->ForceValidFor(PRELOAD_FORCE_VALID_DEFAULT); before opening output stream. > and note that there can be only one output stream opened on an entry at one > time. Yes > > @@ +126,5 @@ > > + return NS_OK; > > + > > +cache_error: > > + MOZ_ASSERT(mCacheEntry); > > + mCacheEntry->ForceValidFor(PRELOAD_FORCE_VALID_DEFAULT); > > (0) ? if I set it to 0 it will not be valid for a real load that comes a bit later? > > @@ +234,5 @@ > > + mState = PRELOAD_DO_NOT_STORE; > > + return rv; > > + } > > + nsAutoCString extension; > > + extension.Append(nsPrintfCString("preload=u%" PRIu64, mTopWinId)); > > (I think you can use nsPrintfCString directly, no need to copy) > > @@ +259,5 @@ > > + mHttpChannel->Suspend(); > > + > > + rv = cacheStorage->AsyncOpenURI(uri, extension, > > + nsICacheStorage::OPEN_NORMALLY | > > + nsICacheStorage::CHECK_MULTITHREADED, this); > > as I understand, there is always only one preload per winid+uri, right? > hence, there will never be an entry found - you will always get a new one, > right? if so, you can use OpenTruncate and get the entry right away. it > will be empty, you can then set metadata on it and write to its output > stream the same way as you do when obtained with OnCacheEntryAvailable. > yes there is only one preload per winid+uri. So if I call OpenTruncate i do not need to wait for OnCacheEntryAvailable? > @@ +312,5 @@ > > + if (mState == PRELOAD_DO_NOT_STORE){ > > + return NS_ERROR_ABORT; > > + } > > + > > + mHttpChannel->Resume(); > > calling resume is super-sensitive. i'd rather see a flag "resume call > needed" that when true, you will call resume() where you want and then drop > that flag. dtor should assert "resume call needed == false" then. > I can add check for mState==PRELOAD_WAITING_FOR_CACHE. It is suspended only in that state. > @@ +385,5 @@ > > + nsCOMPtr<nsIHttpChannelInternal> httpChannelInternal = > > + do_QueryInterface(mRedirectChannel); > > + if (httpChannelInternal) { > > + httpChannelInternal->SetPreload(true); > > + httpChannelInternal->SetPreloadWindowId(mTopWinId); > > will this preload get canceled when the link is removed? > is the original channel preload mem cache entry made available (set "having > metadata") to real channels? > what about a redirect to the same uri? something we allow and also is > sometimes common: redirect to self sets different cookies, sometimes even in > a loop ; the redirect to self even more suggests using OpenTrancate to get > the mem-cache preload entry, OpenTrance also always dooms any existing entry > to create a new one > The redirect was not working properly in this patch. I could not test it before submitting the patche because of winIds problem. I will write some comments in the new patch how redirect work. > @@ +570,5 @@ > > + > > + if (!strcmp(aTopic, NS_XPCOM_SHUTDOWN_OBSERVER_ID)) { > > + RemoveAll(); > > + mShutdown = true; > > + } else if (!strcmp(aTopic, NS_PREFBRANCH_PREFCHANGE_TOPIC_ID)) { > > don't like Preferences::AddBoolVarCache ? ;) it's actually preferred since > the preferences getters (GetBool) are doing a hash lookup that is expensive > > ::: uriloader/prefetch/nsPreloadServiceBackend.h > @@ +74,5 @@ > > + bool Take(); > > + > > + RefPtr<nsPreloadServiceBackend> mService; > > + nsCOMPtr<nsIHttpChannel> mHttpChannel; > > + nsCOMPtr<nsIHttpChannel> mRedirectChannel; > > ok, preloads are IIUC allowed only for http, so this is OK. note that we > generally allow http redirs to different schemes. yes I know, but rel=preload will not work with that. the channel will be canceled.

wrong bug :)

(In reply to Dragana Damjanovic [:dragana] from comment #16) > wrong bug :) good try! :D

(In reply to Dragana Damjanovic [:dragana] from comment #15) > This trying/failing untill I got it right, I think. > I call DoAddCacheEntryHeaders and > mCacheEntry->ForceValidFor(PRELOAD_FORCE_VALID_DEFAULT); before opening > output stream. I think DoAddCacheENtryHeadrs will call metadataready(), so opening output here will have no effect.. that was just FYI anyway. > > > + mCacheEntry->ForceValidFor(PRELOAD_FORCE_VALID_DEFAULT); > > > > (0) ? > > if I set it to 0 it will not be valid for a real load that comes a bit later? you are dooming the entry right under, so it looked to me like you want to remove it. doomed entries are no longer accessible via asyncOpenURI > > stream the same way as you do when obtained with OnCacheEntryAvailable. > > > > yes there is only one preload per winid+uri. > > So if I call OpenTruncate i do not need to wait for OnCacheEntryAvailable? No! that's the nice thing. that method has an entry as its out param. you can work with it immediately. > > > @@ +312,5 @@ > > > + if (mState == PRELOAD_DO_NOT_STORE){ > > > + return NS_ERROR_ABORT; > > > + } > > > + > > > + mHttpChannel->Resume(); > > > > calling resume is super-sensitive. i'd rather see a flag "resume call > > needed" that when true, you will call resume() where you want and then drop > > that flag. dtor should assert "resume call needed == false" then. > > > > I can add check for mState==PRELOAD_WAITING_FOR_CACHE. It is suspended only > in that state. OK, I would prefer a specific flag, but if you can 100% guarantee that this WILL work, then go ahead... I'm always nervous around suspend/resume calls being not clean-and-clear ensured 1-1. > > > @@ +385,5 @@ > > > + nsCOMPtr<nsIHttpChannelInternal> httpChannelInternal = > > > + do_QueryInterface(mRedirectChannel); > > > + if (httpChannelInternal) { > > > + httpChannelInternal->SetPreload(true); > > > + httpChannelInternal->SetPreloadWindowId(mTopWinId); > > > > will this preload get canceled when the link is removed? > > is the original channel preload mem cache entry made available (set "having > > metadata") to real channels? > > what about a redirect to the same uri? something we allow and also is > > sometimes common: redirect to self sets different cookies, sometimes even in > > a loop ; the redirect to self even more suggests using OpenTrancate to get > > the mem-cache preload entry, OpenTrance also always dooms any existing entry "OpenTruncate" (yeah, radio23 was good today!!!) > > ok, preloads are IIUC allowed only for http, so this is OK. note that we > > generally allow http redirs to different schemes. > > yes I know, but rel=preload will not work with that. the channel will be > canceled. good.

I have added a timer.A preload resource will be valid for 60s pref to control the setting is network.preload_valid_time. The timer is not precise but I do not thin that it is important ans save as going through the whole list every time it needs to be set. I decided to scope preload to tab inner winId. Since we always go through httpChannel all necessary security checks will be perform and there is no risk ind scoping it in this way. Honza, a preloadBackenNode will drop reference to its cacheEntry if it does not need it any more. So the reference can be dropped before all data is written. I think this is ok, tee will still hold reference to it.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=018357736984b6bd2426e13a5eae9e51b6004dc1 This feature is only e10s feature. The try run looks ok only on non-e10s test are failing. This run only on win7. I will update the test. The web-platform-tests are coming in a separate patch.

Comment on attachment 8903471 [details] [diff] [review] bug_preload_nocache_v1.patch Dropping review, i need to make a small update to make sure preload works the same as before for non-e10s.

This patch reverts web-platform-tests changes needed for the cache only preload.

I have added a timer.A preload resource will be valid for 60s pref to control the setting is network.preload_valid_time. The timer is not precise but I do not thin that it is important ans save as going through the whole list every time it needs to be set. I decided to scope preload to tab inner winId. Since we always go through httpChannel all necessary security checks will be perform and there is no risk ind scoping it in this way. Honza, a preloadBackenNode will drop reference to its cacheEntry if it does not need it any more. So the reference can be dropped before all data is written. I think this is ok, tee will still hold reference to it.

For getting winId I am using the calles suggested in comment 12. Olli wrote that is ok that this is only working for e10s. For non-e10s we will do the same thing as until now, i.e. preload will work for only cacheable resources. Therefore some of the web-platform tests will fail. This patch adds these exceptions.

Comment on attachment 8903526 [details] [diff] [review] bug_preload_nocache_v1.patch Review of attachment 8903526 [details] [diff] [review]: ----------------------------------------------------------------- r+ BUT the redirects need an updated (we agreed with Dragana on an interdiff for a quick r) ::: netwerk/protocol/http/HttpBaseChannel.h @@ +662,5 @@ > nsCString mMatchedProvider; > nsCString mMatchedPrefix; > + > + bool mPreload; > + nsCOMPtr<nsIPreloadNodeBackend> mPreloadNode; why does a child channel may have a preload node? ::: netwerk/protocol/http/nsHttpChannel.cpp @@ +553,5 @@ > NS_ENSURE_SUCCESS(rv,rv); > rv = OpenCacheEntry(isHttps); > > + if (mPreload && !mPreloadNode) { > + // Make a PreloadBachendNode. The PreloadBachendNode takes care of PreloadBackendNode @@ +5884,5 @@ > + if (mPreload && mPreloadNode) { > + nsCOMPtr<nsIHttpChannelInternal> httpChannelInternal = > + do_QueryInterface(mRedirectChannel); > + httpChannelInternal->SetPreload(true); > + httpChannelInternal->SetPreloadNode(mPreloadNode); this is strange. the redir target channel gets the same preload node for the same URL as the original channel, but the target channel may actually load a completely different URL. this is something I was pointing at during the feedback, not sure what was the outcome. @@ +5886,5 @@ > + do_QueryInterface(mRedirectChannel); > + httpChannelInternal->SetPreload(true); > + httpChannelInternal->SetPreloadNode(mPreloadNode); > + } > + tws, 2 blank lines ::: uriloader/prefetch/nsIPreloadServiceBackend.idl @@ +51,5 @@ > + out nsIStreamListener aOut); > + > + void doneWriting(in nsresult aStatus); > + > + void doneReading(); would use comments too ::: uriloader/prefetch/nsPrefetchService.cpp @@ +248,5 @@ > return NS_BINDING_ABORTED; > } > > + // If we are using mem-only cache we should not cancel channel if a > + // resource is not cacheable. would be good to say also why @@ +447,5 @@ > + Preferences::AddWeakObserver(this, PRELOAD_MEM_PREF); > + } else { > + mPreloadToMem = false; > + mPreloadE10s = true; > + } isn't this backwards? @@ +485,3 @@ > { > + nsRefPtrHashtable<nsCStringHashKey, nsPrefetchNode> *entry1; > + mPreloadNodes.Get(aFinished->mTopWinId, &entry1); what's this for? @@ +692,5 @@ > } > } > > void > +nsPrefetchService::StopCurrentPrefetchs() StopCurrentPrefetch_e_s ? @@ +1051,5 @@ > NS_IMETHODIMP > +nsPrefetchService::CancelPreloadURI(nsIURI* aURI, > + nsIDOMNode* aSource) > +{ > + if (LOG_ENABLED()) { probably no need :) @@ +1283,5 @@ > + mozilla::services::GetObserverService(); > + if (observerService) { > + observerService->RemoveObserver(this, DOM_WINDOW_DESTROYED_TOPIC); > + } > + } I'm just curious what may happen when some changes this pref at runtime while this machine is some middle state. but that is not critical... ::: uriloader/prefetch/nsPrefetchService.h @@ +86,5 @@ > bool mHaveProcessed; > bool mPrefetchDisabled; > bool mPreloadDisabled; > + bool mPreloadToMem; > + bool mPreloadE10s; I don't follow how these two actually work. comment needed? ::: uriloader/prefetch/nsPreloadServiceBackend.cpp @@ +27,5 @@ > +// > +// this enables LogLevel::Debug level information and places all output in > +// the file prefetch.log > +// > +//extern LazyLogModule gPrefetchLog; this is a bit confusing, if not needed, please remove this line @@ +142,5 @@ > + this, mState, (uint32_t)aStatus)); > + > + if (mState == PRELOAD_WAITING_FOR_START) { > + mCacheEntry->AsyncDoom(nullptr); > + } isn't this redundant? you do the same few lines below @@ +145,5 @@ > + mCacheEntry->AsyncDoom(nullptr); > + } > + if (mState == PRELOAD_DO_NOT_STORE) { > + return NS_OK; > + } else if (mState == PRELOAD_WAITING_FOR_START) { else after return @@ +151,5 @@ > + MOZ_ASSERT(mService); > + // RemovePreload will set mState to PRELOAD_DO_NOT_STORE. > + mService->RemovePreload(mURISpec, mTopWinId); > + return NS_OK; > + } else if (NS_FAILED(aStatus)) { here too @@ +159,5 @@ > + return NS_OK; > + } > + > + mState = PRELOAD_DONE; > + return NS_OK; a bit hard to orient myself in this condition tree... @@ +182,5 @@ > + > +bool > +nsPreloadNodeBackend::Take() > +{ > + LOG(("nsPreloadNodeBackend::Taken [this=%p state=%d]", this, mState)); update the logged method name @@ +214,5 @@ > + if ((mState == PRELOAD_WAITING_FOR_START) || > + (mState == PRELOAD_DATA) || > + (mState == PRELOAD_DONE)) { > + MOZ_ASSERT(mCacheEntry); > + mCacheEntry->ForceValidFor(0); no dooming? (not saying it's wrong, just would be good to explain why not) @@ +282,5 @@ > +} > + > +nsPreloadServiceBackend::~nsPreloadServiceBackend() > +{ > + RemoveAll(); since the nodes backrefer this service I don't think this has a meaning. @@ +313,5 @@ > +nsPreloadServiceBackend::InitPreload(nsIURI *aUri, uint64_t aWinId, > + nsILoadContextInfo *aLoadInfo, > + nsIPreloadNodeBackend **aNode) > +{ > + NS_ENSURE_ARG_POINTER(aNode); assert thread, please do so for every method that accesses mPreloadRequests @@ +325,5 @@ > + return NS_ERROR_NOT_AVAILABLE; > + } > + > + nsAutoCString spec; > + aUri->GetSpec(spec); we have changed GetSpec to always return the same representation now? I used to always get the spec string via GetAsciiSpec (valentin knows now which is the correct one to use, but looking to the cache2 code there is still getAsciiSpec, so it has not been updated to use GetSpec) @@ +338,5 @@ > + MOZ_ASSERT(false); > + return NS_ERROR_FAILURE; > + } > + } > + tws @@ +422,5 @@ > + nsRefPtrHashtable<nsCStringHashKey, nsPreloadNodeBackend> *entry; > + if ((mPreloadRequests.Get(aTopWinId, &entry))) { > + RefPtr<nsPreloadNodeBackend> node; > + if (entry->Get(aURISpec, getter_AddRefs(node))) { > + if (node && node->Take()) { could it ever happen the node were null when Get returned true? ::: uriloader/prefetch/nsPreloadServiceBackend.h @@ +116,5 @@ > + * cache.) > + */ > + enum { > + PRELOAD_NOT_INIT, > + PRELOAD_WAITING_FOR_START, // We have a mam-only preload cache entry and we mem-only

This patch fixes redirects. now all redirects are saved in mem-only cache. A real load is going to follow redirects as any http channel (all security stuff, principals will be the same as a load from the network). The redirected resources are only available if the main preload resource is access. (a real load will access the main preloaded mem-only cache entry and it will be redirected to the next one that will be in the mem-only cache. The redirected resources are not available without accessing the first one.)

Comment on attachment 8903526 [details] [diff] [review] bug_preload_nocache_v1.patch >+++ b/dom/base/test/mochitest.ini >@@ -192,16 +192,19 @@ support-files = > orientationcommon.js > plugin.js > script-1_bug597345.sjs > script-2_bug597345.js > script_bug602838.sjs > send_gzip_content.sjs > somedatas.resource > somedatas.resource^headers^ >+ square_redirect.png >+ square_redirect.png^headers^ >+ square.png Tests really should be in a separate patch, especially when the patch is as massive as this one. >+mozilla::ipc::IPCResult >+ContentParent::RecvCancelPreload(const nsCString& aUriSpec, >+ const uint64_t& aTopWinId) Ok, to we're dealing with top window id here. > >+ nsCOMPtr<nsIPreloadServiceBackend> mPreloadServiceBackend; >+ nsIPreloadServiceBackend * GetPreloadServiceBackend(); Nit, extra space before * > // Classified channel's matched information > nsCString mMatchedList; > nsCString mMatchedProvider; > nsCString mMatchedPrefix; >+ >+ bool mPreload; >+ nsCOMPtr<nsIPreloadNodeBackend> mPreloadNode; Don't you want to define mPreload in the same places as other boolean-like member variables to reduce increase of sizeof(HttpBaseChannel) > >+ if (mPreload && !mPreloadNode) { >+ // Make a PreloadBachendNode. The PreloadBachendNode takes care of Bachend? >+ // a mem-only cache entry for a rel=preload. >+ // redirected channel will already have an entry. >+ nsIPreloadServiceBackend *preloadServiceBackend = gHttpHandler->GetPreloadServiceBackend(); Nit, * goes with the type >diff --git a/testing/web-platform/meta/preload/single-download-preload.html.ini b/testing/web-platform/meta/preload/single-download-preload.html.ini >new file mode 100644 >--- /dev/null >+++ b/testing/web-platform/meta/preload/single-download-preload.html.ini >@@ -0,0 +1,4 @@ >+[single-download-preload.html] >+ type: testharness >+ [Makes sure that preloaded resources are not downloaded again when used] >+ expected: FAIL Why is this failing now? > > /** >- * Cancel prefetch or preload for a nsIDomNode. >+ * Cancel prefetch for a nsIDomNode. Want to fix the typo. It is nsIDOMNode, not nsIDomNode >+[builtinclass, uuid(06faa986-b560-4c7f-a538-c9b198c59611)] >+interface nsIPreloadServiceBackend : nsISupports >+{ >+ /** >+ * Initiate a mem-only preload cache entry. >+ * The function returns a node that holds the cache entry. >+ * All nodes are also kept in a hash table. >+ */ >+ void initPreload(in nsIURI aUri, >+ in uint64_t aWinId, Please document what kind of window ID this is supposed to be. Inner window or outer window? top level window? >+ in nsILoadContextInfo aLoadInfo, >+ out nsIPreloadNodeBackend aNode); Just make the method return nsIPreloadNodeBackend >+ >+ /** >+ * Lookup if a mem-only preload cache entry exist. >+ * A http request (not a rel=preload) calls this function to find if a >+ * preloaded resource exist. >+ */ >+ void lookupPreload(in ACString aURISpec, >+ in uint64_t aWinId, >+ out nsIPreloadNodeBackend aNode); Ditto >+ >+ /** >+ * Removes a preloaded resource. >+ */ >+ void RemovePreload(in ACString aURISpec, >+ in uint64_t aWinId); And here too, what kind of Id is that? >+[builtinclass, uuid(c8df12e1-dd7a-4c16-bb8b-6fa4a6f55476)] >+interface nsIPreloadNodeBackend : nsISupports >+{ >+ void attachPreloadCache(in nsIHttpChannel aHttpChannel, >+ in nsIStreamListener aListener, >+ out nsIStreamListener aOut); just return nsIStreamListener >+void >+nsPrefetchService::DispatchEventToANode(nsIDOMNode *aNode, bool aSuccess) Nit, * goes with the type >+ if (!mPreloadE10s) { >+ nsCOMPtr<nsINode> inode = do_QueryInterface(aSource); >+ nsCOMPtr<nsIDocument> ownerDoc = inode->GetOwnerDocument(); Use inode->OwnerDoc() which never returns null >+ uint64_t topWinId = 0; >+ if (ownerDoc && ownerDoc->GetDocShell()) { >+ nsCOMPtr<nsITabChild> iTabChild = ownerDoc->GetDocShell()->GetTabChild(); >+ if (iTabChild) { >+ mozilla::dom::TabChild* tabChild = static_cast<mozilla::dom::TabChild*>(iTabChild.get()); >+ nsCOMPtr<nsIDocument> document = tabChild->GetDocument(); >+ if (document) { >+ topWinId = document->InnerWindowID(); >+ } >+ } >+ } else { >+ nsCOMPtr<nsIDocument> doc = do_QueryInterface(inode); >+ if (doc) { >+ topWinId = doc->InnerWindowID(); >+ } >+ } I don't understand this stuff. Why do we care about document nodes which don't have docshell? And that topWinId wouldn't be topWinId, but some inner window id >+nsPrefetchService::CancelPreloadURI(nsIURI* aURI, >+ nsIDOMNode* aSource) >+{ >+ if (LOG_ENABLED()) { >+ LOG(("CancelPreloadURI [%s]\n", aURI->GetSpecOrDefault().get())); >+ } >+ >+ uint64_t topWinId = 0; >+ if (!mPreloadE10s) { >+ nsCOMPtr<nsINode> inode = do_QueryInterface(aSource); >+ nsCOMPtr<nsIDocument> ownerDoc = inode->GetOwnerDocument(); OwnerDoc() >+ uint64_t topWinId = 0; >+ if (ownerDoc && ownerDoc->GetDocShell)) { >+ nsCOMPtr<nsITabChild> iTabChild = ownerDoc->GetDocShell()->GetTabChild(); >+ if (iTabChild) { >+ mozilla::dom::TabChild* tabChild = static_cast<mozilla::dom::TabChild*>(iTabChild.get()); >+ nsCOMPtr<nsIDocument> document = tabChild->GetDocument(); >+ if (document) { >+ topWinId = document->InnerWindowID(); >+ } >+ } >+ } else { >+ nsCOMPtr<nsIDocument> doc = do_QueryInterface(inode); >+ if (doc) { >+ topWinId = doc->InnerWindowID(); >+ } >+ } And don't understand this stuff here This was preliminary read-through, I'll need to read this couple of times. This is after all rather big patch. Could you fix the issues and upload a new version. I especially would like to see better documentation about window ID usage and maybe even some assertions if the ids are supposed to be top level window IDs

(In reply to Olli Pettay [:smaug] from comment #28) > Comment on attachment 8903526 [details] [diff] [review] > bug_preload_nocache_v1.patch > > > >diff --git a/testing/web-platform/meta/preload/single-download-preload.html.ini b/testing/web-platform/meta/preload/single-download-preload.html.ini > >new file mode 100644 > >--- /dev/null > >+++ b/testing/web-platform/meta/preload/single-download-preload.html.ini > >@@ -0,0 +1,4 @@ > >+[single-download-preload.html] > >+ type: testharness > >+ [Makes sure that preloaded resources are not downloaded again when used] > >+ expected: FAIL > Why is this failing now? This is bug 1393540. > >+ uint64_t topWinId = 0; > >+ if (ownerDoc && ownerDoc->GetDocShell()) { > >+ nsCOMPtr<nsITabChild> iTabChild = ownerDoc->GetDocShell()->GetTabChild(); > >+ if (iTabChild) { > >+ mozilla::dom::TabChild* tabChild = static_cast<mozilla::dom::TabChild*>(iTabChild.get()); > >+ nsCOMPtr<nsIDocument> document = tabChild->GetDocument(); > >+ if (document) { > >+ topWinId = document->InnerWindowID(); > >+ } > >+ } > >+ } else { > >+ nsCOMPtr<nsIDocument> doc = do_QueryInterface(inode); > >+ if (doc) { > >+ topWinId = doc->InnerWindowID(); > >+ } > >+ } > I don't understand this stuff. Why do we care about document nodes which > don't have docshell? > And that topWinId wouldn't be topWinId, but some inner window id > This is rest form my previous implementation that returns null if the node is the ownerDoc (this was happening for headers)

(In reply to Olli Pettay [:smaug] from comment #28) > Comment on attachment 8903526 [details] [diff] [review] > bug_preload_nocache_v1.patch > > > >+ // a mem-only cache entry for a rel=preload. > >+ // redirected channel will already have an entry. > >+ nsIPreloadServiceBackend *preloadServiceBackend = gHttpHandler->GetPreloadServiceBackend(); > Nit, * goes with the type > this is more in necko style coding, at least all in /urlloader/prefetch/*, so * goes with variable name.

A separate patch for tests.

Comment on attachment 8904452 [details] [diff] [review] bug_preload_nocache_test_change.patch Would be nice to add some comment to single-download-preload.html.ini that the failure is about bug 1393540

I'd still argue that new files should follow Mozilla coding style, not some random inconsistent style.

Comment on attachment 8904453 [details] [diff] [review] bug_preload_nocache_v2.patch >+++ b/netwerk/protocol/http/HttpBaseChannel.cpp >@@ -207,16 +207,17 @@ HttpBaseChannel::HttpBaseChannel() > , mContentWindowId(0) > , mTopLevelOuterContentWindowId(0) > , mRequireCORSPreflight(false) > , mReportCollector(new ConsoleReportCollector()) > , mAltDataLength(0) > , mForceMainDocumentChannel(false) > , mIsTrackingResource(false) > , mLastRedirectFlags(0) >+ , mPreload(false) You should see a warning about mPreload initialized after other member variables which are defined after it. Move the initialization to the right place > >+ nsCOMPtr<nsIPreloadNodeBackend> mPreloadNode; So the interface is nsIPreloadNodeBackend, but some comments talk about PreloadBackendNode. Fix the comments. >-nsPrefetchService::RemoveNodeAndMaybeStartNextPrefetchURI(nsPrefetchNode *aFinished) >+nsPrefetchService::RemoveNodeAndMaybeStartNextPrefetchURI(nsPrefetchNode *aFinished, >+ bool aSucceeded) > { > if (aFinished) { >- mCurrentNodes.RemoveElement(aFinished); >+ if (!aFinished->mPreload) { >+ mPrefetchingNodes.RemoveElement(aFinished); >+ } else if (!aSucceeded || !aFinished->mPreloadToMem) { >+ nsRefPtrHashtable<nsCStringHashKey, nsPrefetchNode> *entry; >+ if (mPreloadNodes.Get(aFinished->mTopWinId, &entry)) { >+ nsAutoCString spec; >+ aFinished->mURI->GetSpec(spec); >+ SendCancelPreload(spec, aFinished->mTopWinId); >+ entry->Remove(spec); >+ if (!entry->Count()) { >+ mPreloadNodes.Remove(aFinished->mTopWinId); >+ } >+ } >+ } > } This is weird. The method name talks about Prefetch, but most of the implementation talks about preload. Also, why !aFinished->mPreloadToMem? Please add some comments, and perhaps rename the method? > if (aPolicyType == nsIContentPolicy::TYPE_INVALID) { >- nsCOMPtr<nsINode> domNode = do_QueryInterface(aSource); >- if (domNode && domNode->IsInComposedDoc()) { >- RefPtr<AsyncEventDispatcher> asyncDispatcher = >- new AsyncEventDispatcher(//domNode->OwnerDoc(), >- domNode, >- NS_LITERAL_STRING("error"), >- /* aCanBubble = */ false, >- /* aCancelable = */ false); >- asyncDispatcher->RunDOMEventWhenSafe(); >- } >+ DispatchEventToANode(aSource, false); DispatchEventToANode uses PostDOMEvent(), the old code RunDOMEventWhenSafe(). Those are very different things. Please explain why the change. (I have the gut feeling that new behavior is the correct one) >+ nsRefPtrHashtable<nsCStringHashKey, nsPrefetchNode> *entry; >+ if (mPreloadNodes.Get(topWinId, &entry)) { >+ nsPrefetchNode *node = entry->GetWeak(spec); >+ if (node) { >+ if (node->mPolicyType != aPolicyType) { >+ DispatchEventToANode(aSource, false); Why do we dispatch error event in this case? (this will need still couple of read-throughs)

(In reply to Olli Pettay [:smaug] from comment #35) > Comment on attachment 8904453 [details] [diff] [review] > bug_preload_nocache_v2.patch > >-nsPrefetchService::RemoveNodeAndMaybeStartNextPrefetchURI(nsPrefetchNode *aFinished) > >+nsPrefetchService::RemoveNodeAndMaybeStartNextPrefetchURI(nsPrefetchNode *aFinished, > >+ bool aSucceeded) > > { > > if (aFinished) { > >- mCurrentNodes.RemoveElement(aFinished); > >+ if (!aFinished->mPreload) { > >+ mPrefetchingNodes.RemoveElement(aFinished); > >+ } else if (!aSucceeded || !aFinished->mPreloadToMem) { > >+ nsRefPtrHashtable<nsCStringHashKey, nsPrefetchNode> *entry; > >+ if (mPreloadNodes.Get(aFinished->mTopWinId, &entry)) { > >+ nsAutoCString spec; > >+ aFinished->mURI->GetSpec(spec); > >+ SendCancelPreload(spec, aFinished->mTopWinId); > >+ entry->Remove(spec); > >+ if (!entry->Count()) { > >+ mPreloadNodes.Remove(aFinished->mTopWinId); > >+ } > >+ } > >+ } > > } > This is weird. The method name talks about Prefetch, but most of the > implementation talks about preload. > Also, why !aFinished->mPreloadToMem? > Please add some comments, and perhaps rename the method? > I have split this function. mPreloadToMem is true if the pref that control use of mem-only cache is turned off. > > if (aPolicyType == nsIContentPolicy::TYPE_INVALID) { > >- nsCOMPtr<nsINode> domNode = do_QueryInterface(aSource); > >- if (domNode && domNode->IsInComposedDoc()) { > >- RefPtr<AsyncEventDispatcher> asyncDispatcher = > >- new AsyncEventDispatcher(//domNode->OwnerDoc(), > >- domNode, > >- NS_LITERAL_STRING("error"), > >- /* aCanBubble = */ false, > >- /* aCancelable = */ false); > >- asyncDispatcher->RunDOMEventWhenSafe(); > >- } > >+ DispatchEventToANode(aSource, false); > DispatchEventToANode uses PostDOMEvent(), the old code RunDOMEventWhenSafe(). > Those are very different things. Please explain why the change. (I have the > gut feeling that new behavior is the correct one) > > Currently we use both, I just fix that. I think it should have always been PostDOMEvent. > >+ nsRefPtrHashtable<nsCStringHashKey, nsPrefetchNode> *entry; > >+ if (mPreloadNodes.Get(topWinId, &entry)) { > >+ nsPrefetchNode *node = entry->GetWeak(spec); > >+ if (node) { > >+ if (node->mPolicyType != aPolicyType) { > >+ DispatchEventToANode(aSource, false); > Why do we dispatch error event in this case? > pre the spec this case should dispatch an error on the node.

rebased. Honza is on vacation.

Comment on attachment 8904902 [details] [diff] [review] bug_preload_nocache_v2.patch > mService->NotifyLoadCompleted(this); > mService->DispatchEvent(this, mShouldFireLoadEvent || NS_SUCCEEDED(aStatus)); >- mService->RemoveNodeAndMaybeStartNextPrefetchURI(this); >+ if (!mPreload) { >+ mService->RemoveNodeAndMaybeStartNextPrefetchURI(this, NS_SUCCEEDED(aStatus)); >+ } else if (!aSucceeded || !aFinished->mPreloadToMem) { >+ // If the preload is not successful or preload to the mem-only cache is turned off, >+ // remove preload from the hashTable. >+ mService->RemovePreloadURI(this); And we don't need to do anything like "MaybeStartNext..." in preload case? > nsPrefetchService::StopPrefetching() > { > mStopCount++; > > LOG(("StopPrefetching [stopcount=%d]\n", mStopCount)); > > // When we start a load, we need to stop all prefetches that has been >- // added by the old load, therefore call StopAll only at the moment we >- // switch to a new page load (i.e. mStopCount == 1). >+ // added by the old load, therefore call StopCurrentPrefetches and >+ // StopCurrentPreloads only at the moment we switch to a new page load >+ // (i.e. mStopCount == 1). >+ // On e10s we use windowId to scope preloads and the preloads for a page >+ // we be removed when the corresponding page is removed/preloaded. for a top level page > NS_IMETHODIMP >+nsPrefetchService::CancelPreloadURI(nsIURI* aURI, >+ nsIDOMNode* aSource) >+{ >+ LOG(("CancelPreloadURI [%s]\n", aURI->GetSpecOrDefault().get())); >+ >+ uint64_t topWinId = 0; >+ if (mPreloadIsE10s) { >+ nsCOMPtr<nsINode> inode = do_QueryInterface(aSource); >+ nsCOMPtr<nsIDocument> ownerDoc = inode->OwnerDoc(); >+ if (ownerDoc->GetDocShell()) { >+ nsCOMPtr<nsITabChild> iTabChild = ownerDoc->GetDocShell()->GetTabChild(); >+ if (iTabChild) { >+ mozilla::dom::TabChild* tabChild = static_cast<mozilla::dom::TabChild*>(iTabChild.get()); >+ nsCOMPtr<nsIDocument> document = tabChild->GetDocument(); >+ if (document) { >+ topWinId = document->InnerWindowID(); >+ } >+ } >+ } >+ if (!topWinId) { >+ return NS_OK; So this may happen rather easily if iframe is doing some preloading and that iframe is removed from document. Why is it ok to keep doing the preload? >+ } else if (!strcmp(aTopic, DOM_WINDOW_DESTROYED_TOPIC)) { >+ if (mPreloadIsE10s) { >+ nsCOMPtr<nsPIDOMWindowInner> window = do_QueryInterface(aSubject); >+ LOG(("DOM_WINDOW_DESTROYED_TOPIC winId=%" PRIx64 ".", window->WindowID())); >+ CancelPreload(window->WindowID()); >+ } I don't quite understand why we're interested in only the top level window IDs. What if preload was initiated from an iframe and that window is destroyed? >+ enum { >+ PRELOAD_NOT_INIT, >+ PRELOAD_WAITING_FOR_START, // We have a mem-only preload cache entry and we >+ // now wait for HttpChannel::OnStartRequest to >+ // attach the cache listener. >+ PRELOAD_DATA, // We have a mem-only preload cache entry and a cache >+ // listener. >+ PRELOAD_DONE, // DoneWriting is called. The cache entry is filled. >+ PRELOAD_DO_NOT_STORE // The node is in this state when preload is canceled. >+ } mState; Nit { after enum goes to its own line. >+}; >+ >+#endif // !nsPreloadServiceBackend_h__

(In reply to Olli Pettay [:smaug] from comment #39) > Comment on attachment 8904902 [details] [diff] [review] > bug_preload_nocache_v2.patch > > > mService->NotifyLoadCompleted(this); > > mService->DispatchEvent(this, mShouldFireLoadEvent || NS_SUCCEEDED(aStatus)); > >- mService->RemoveNodeAndMaybeStartNextPrefetchURI(this); > >+ if (!mPreload) { > >+ mService->RemoveNodeAndMaybeStartNextPrefetchURI(this, NS_SUCCEEDED(aStatus)); > >+ } else if (!aSucceeded || !aFinished->mPreloadToMem) { > >+ // If the preload is not successful or preload to the mem-only cache is turned off, > >+ // remove preload from the hashTable. > >+ mService->RemovePreloadURI(this); > And we don't need to do anything like "MaybeStartNext..." in preload case? > No we do not need it. This is only for prefetch. > > nsPrefetchService::StopPrefetching() > > { > > mStopCount++; > > > > LOG(("StopPrefetching [stopcount=%d]\n", mStopCount)); > > > > // When we start a load, we need to stop all prefetches that has been > >- // added by the old load, therefore call StopAll only at the moment we > >- // switch to a new page load (i.e. mStopCount == 1). > >+ // added by the old load, therefore call StopCurrentPrefetches and > >+ // StopCurrentPreloads only at the moment we switch to a new page load > >+ // (i.e. mStopCount == 1). > >+ // On e10s we use windowId to scope preloads and the preloads for a page > >+ // we be removed when the corresponding page is removed/preloaded. > for a top level page > > > NS_IMETHODIMP > >+nsPrefetchService::CancelPreloadURI(nsIURI* aURI, > >+ nsIDOMNode* aSource) > >+{ > >+ LOG(("CancelPreloadURI [%s]\n", aURI->GetSpecOrDefault().get())); > >+ > >+ uint64_t topWinId = 0; > >+ if (mPreloadIsE10s) { > >+ nsCOMPtr<nsINode> inode = do_QueryInterface(aSource); > >+ nsCOMPtr<nsIDocument> ownerDoc = inode->OwnerDoc(); > >+ if (ownerDoc->GetDocShell()) { > >+ nsCOMPtr<nsITabChild> iTabChild = ownerDoc->GetDocShell()->GetTabChild(); > >+ if (iTabChild) { > >+ mozilla::dom::TabChild* tabChild = static_cast<mozilla::dom::TabChild*>(iTabChild.get()); > >+ nsCOMPtr<nsIDocument> document = tabChild->GetDocument(); > >+ if (document) { > >+ topWinId = document->InnerWindowID(); > >+ } > >+ } > >+ } > >+ if (!topWinId) { > >+ return NS_OK; > So this may happen rather easily if iframe is doing some preloading and that > iframe is removed from document. Why is it ok to keep doing the preload? > I could cancel it but I do not know how to get the winId. I decided to scope preload on top-level window ids because we perform all necessary security checks so there can not be a security problem if we use preload from an iframe on the top level doc. Preloads will be removed after some time so they will not stay for ever. I could change this to use non-top-level window id, but I would need help because I do not know how to get the proper win Ids? Which winId should I use? and how to detect that it is removed, destroyed. This should also work for Link headers. > >+ } else if (!strcmp(aTopic, DOM_WINDOW_DESTROYED_TOPIC)) { > >+ if (mPreloadIsE10s) { > >+ nsCOMPtr<nsPIDOMWindowInner> window = do_QueryInterface(aSubject); > >+ LOG(("DOM_WINDOW_DESTROYED_TOPIC winId=%" PRIx64 ".", window->WindowID())); > >+ CancelPreload(window->WindowID()); > >+ } > I don't quite understand why we're interested in only the top level window > IDs. > What if preload was initiated from an iframe and that window is destroyed? > > >+ enum { > >+ PRELOAD_NOT_INIT, > >+ PRELOAD_WAITING_FOR_START, // We have a mem-only preload cache entry and we > >+ // now wait for HttpChannel::OnStartRequest to > >+ // attach the cache listener. > >+ PRELOAD_DATA, // We have a mem-only preload cache entry and a cache > >+ // listener. > >+ PRELOAD_DONE, // DoneWriting is called. The cache entry is filled. > >+ PRELOAD_DO_NOT_STORE // The node is in this state when preload is canceled. > >+ } mState; > > Nit { after enum goes to its own line. > > > >+}; > >+ > >+#endif // !nsPreloadServiceBackend_h__

element->OwnerDoc()->InnerWindowID() gives the id. And all windows fire the same notification which you already observe in your patch.

(In reply to Olli Pettay [:smaug] from comment #41) > element->OwnerDoc()->InnerWindowID() gives the id. And all windows fire the > same notification which you already observe in your patch. I will need the same version for winId for each httpChannelChild. I tried couple of possibilities but did not get the right one. I need to query GetCallback() for the right interface. Do you know which one? The main tag windowId is retrieved by code at: https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelChild.cpp#2499

Can't you get back to the element using channel's loadinfo?

rebased.

Comment on attachment 8906520 [details] [diff] [review] bug_preload_nocache_v2.patch >+ >+ // This in inner window of the caller that is used to scope rel=preload-s. >+ uint64_t mPreloadInnerWindowId; So if I read the code right, this ID is passed to a method like InitPreload, but per documentation that method should take only top level inner window IDs. Same with LookupPreload and I guess others too. Yet mPreloadInnerWindowId isn't guaranteed to be top level inner window id. This really needs some tests which trigger preload in iframes and assertions that when top level inner window ID is expected, it is really such. Adding assertions would also self-document the code. Right now it is hard to follow what kind of window ID is expected and where.

Comment on attachment 8906523 [details] [diff] [review] bug_preload_nocache_fix_redirects.patch I don't understand the code and lack of comments doesn't help either. Honza should be back tomorrow, please r? him.

window id is not top window id. I have change that in the previous patch and forgot to update comments, this is done in this update. TA test is in the separate patch.

an additional test.

Comment on attachment 8914785 [details] [diff] [review] bug_preload_nocache_test_part2.patch >+// 2) A frame will preload a resource. The preloaded resource will not be >+// availale for the main doc. available

Comment on attachment 8914784 [details] [diff] [review] bug_preload_nocache_v2.patch Ok, so this is just renaming variables to indicate they aren't about top window but any window. Btw, an interdiff would have made reviewing easier.

Comment on attachment 8914786 [details] [diff] [review] bug_preload_nocache_fix_redirects.patch Review of attachment 8914786 [details] [diff] [review]: ----------------------------------------------------------------- r+ with comments fixed/answered ::: netwerk/protocol/http/nsHttpChannel.cpp @@ +598,5 @@ > + } else { // Only a redirect has already a mPreloadNode. > + MOZ_ASSERT(mLoadFlags & LOAD_REPLACE); > + nsCOMPtr<nsILoadContextInfo> lci = GetLoadContextInfo(this); > + if (NS_FAILED(mPreloadNode->AddRedirect(mURI, lci))) { > + // If this fails we can keep the one we already have and do not "the one" means? @@ +601,5 @@ > + if (NS_FAILED(mPreloadNode->AddRedirect(mURI, lci))) { > + // If this fails we can keep the one we already have and do not > + // add consecutive ones. > + mPreloadNode = nullptr; > + } nit: indent @@ +3961,5 @@ > + "node [this=%p node=%p].", this, mPreloadNode.get())); > + } > + } > + } > + tws @@ +3963,5 @@ > + } > + } > + > + if (mPreloadNode) { > + if (NS_SUCCEEDED(mPreloadNode->CanStartReading(mPreloadRedirectCount))) { the name of the CanStartReading method should rather be Use() or something. This sounds only like a gether, but it actually changes the state. join with && ? @@ +5954,5 @@ > // i.e. after all sinks had been notified > mRedirectChannel->SetOriginalURI(mOriginalURI); > > // If this channel was a preload copy the preloadNode. > + if (mPreloadNode) { the comment may need an update ::: netwerk/protocol/http/nsIHttpChannelInternal.idl @@ +344,5 @@ > * This setter is needed for redirects. > */ > void setPreloadNode(in nsIPreloadNodeBackend preloadNode); > > + void setPreloadRedirectCount(in uint32_t aCount); comments please ::: uriloader/prefetch/nsIPreloadServiceBackend.idl @@ +47,5 @@ > */ > [builtinclass, uuid(c8df12e1-dd7a-4c16-bb8b-6fa4a6f55476)] > interface nsIPreloadNodeBackend : nsISupports > { > + void addRedirect(in nsIURI aUri, in nsILoadContextInfo aLoadInfo); isn't the loadcontextinfo identical for the whole redirect chain? @@ +50,5 @@ > { > + void addRedirect(in nsIURI aUri, in nsILoadContextInfo aLoadInfo); > + > + void initPreloadCache(in nsIHttpChannel aHttpChannel, > + in bool aWillRedirect); as well here, on all methods please ::: uriloader/prefetch/nsPreloadServiceBackend.cpp @@ +181,5 @@ > + (entry->mState == PreloadEntry::PRELOAD_DATA) || > + (entry->mState == PreloadEntry::PRELOAD_DONE)); > + MOZ_ASSERT(entry->mRedirectCount == aPreloadRedirectCount); > + if (entry->mState != PreloadEntry::PRELOAD_DO_NOT_STORE) { > + entry->mCacheEntryTaken = true; just to confirm - only one "real" load can use the preloaded entry, right? is this defined in the spec this way? (just wondering why infinite number of "real" loads can't use it) @@ +224,4 @@ > > + mNextRedirectCount++; > + mPreloadEntries.push_back(std::move(entry)); > + entry.forget(); is the forget() needed? doesn't the move ctor move (clears on |entry|) the pointer? @@ +282,5 @@ > NS_ENSURE_SUCCESS(rv, rv); > > + if (!aRedirectCount) { > + rv = cacheStorage->OpenTruncate(aUri, > + nsPrintfCString("preload=u%" PRIu64 "_%" PRIu32, why the 'u' before '%'? @@ +283,5 @@ > > + if (!aRedirectCount) { > + rv = cacheStorage->OpenTruncate(aUri, > + nsPrintfCString("preload=u%" PRIu64 "_%" PRIu32, > + aWinId, aRedirectCount), aRedirectCount is always 0 @@ +289,5 @@ > + } else { > + > + nsAutoCString ex; > + ex.Append(nsPrintfCString("preload=u%" PRIu64 "_%" PRIu32, aWinId, > + aRedirectCount)); this is identical to the !aRedirectCount case, could we bring it out the branch? why it's not sufficient to just start a new preload for a redirect target URI? If the page happen to preload a resource that is a redirect target of a different preload we load twice. Or is this needed to be spec compliant? @@ +558,3 @@ > iter2.Data()->CancelPreload(); > iter2.Remove(); > + iter2.Next(); why calling Next() here? ::: uriloader/prefetch/nsPreloadServiceBackend.h @@ +71,5 @@ > ~nsPreloadNodeBackend() {} > > friend class nsPreloadServiceBackend; > + nsresult AddLoad(nsIURI *aUri, const nsACString &aSpec, > + nsILoadContextInfo *aLoadInfo); some comments would be nice @@ +83,2 @@ > mozilla::TimeStamp mCreated; > + uint32_t mNextRedirectCount; as well here

(In reply to Honza Bambas (:mayhemer) from comment #54) > Comment on attachment 8914786 [details] [diff] [review] > bug_preload_nocache_fix_redirects.patch > > Review of attachment 8914786 [details] [diff] [review]: > ----------------------------------------------------------------- > > r+ with comments fixed/answered > > ::: netwerk/protocol/http/nsHttpChannel.cpp > @@ +598,5 @@ > > + } else { // Only a redirect has already a mPreloadNode. > > + MOZ_ASSERT(mLoadFlags & LOAD_REPLACE); > > + nsCOMPtr<nsILoadContextInfo> lci = GetLoadContextInfo(this); > > + if (NS_FAILED(mPreloadNode->AddRedirect(mURI, lci))) { > > + // If this fails we can keep the one we already have and do not > > "the one" means? > the previous redirect in the chain. > > @@ +3963,5 @@ > > + } > > + } > > + > > + if (mPreloadNode) { > > + if (NS_SUCCEEDED(mPreloadNode->CanStartReading(mPreloadRedirectCount))) { > > the name of the CanStartReading method should rather be Use() or something. > This sounds only like a gether, but it actually changes the state. > > join with && ? I will change it to StartReading which can return an error as well. > > @@ +5954,5 @@ > > // i.e. after all sinks had been notified > > mRedirectChannel->SetOriginalURI(mOriginalURI); > > > > // If this channel was a preload copy the preloadNode. > > + if (mPreloadNode) { > > the comment may need an update I will update. > > ::: netwerk/protocol/http/nsIHttpChannelInternal.idl > @@ +344,5 @@ > > * This setter is needed for redirects. > > */ > > void setPreloadNode(in nsIPreloadNodeBackend preloadNode); > > > > + void setPreloadRedirectCount(in uint32_t aCount); > > comments please I will add. > > ::: uriloader/prefetch/nsIPreloadServiceBackend.idl > @@ +47,5 @@ > > */ > > [builtinclass, uuid(c8df12e1-dd7a-4c16-bb8b-6fa4a6f55476)] > > interface nsIPreloadNodeBackend : nsISupports > > { > > + void addRedirect(in nsIURI aUri, in nsILoadContextInfo aLoadInfo); > > isn't the loadcontextinfo identical for the whole redirect chain? > yes, but I do not store it in PreloadBackendNode, so I need it every time. > @@ +50,5 @@ > > { > > + void addRedirect(in nsIURI aUri, in nsILoadContextInfo aLoadInfo); > > + > > + void initPreloadCache(in nsIHttpChannel aHttpChannel, > > + in bool aWillRedirect); > > as well here, on all methods please > will add. > ::: uriloader/prefetch/nsPreloadServiceBackend.cpp > @@ +181,5 @@ > > + (entry->mState == PreloadEntry::PRELOAD_DATA) || > > + (entry->mState == PreloadEntry::PRELOAD_DONE)); > > + MOZ_ASSERT(entry->mRedirectCount == aPreloadRedirectCount); > > + if (entry->mState != PreloadEntry::PRELOAD_DO_NOT_STORE) { > > + entry->mCacheEntryTaken = true; > > just to confirm - only one "real" load can use the preloaded entry, right? > is this defined in the spec this way? (just wondering why infinite number > of "real" loads can't use it) > This is still not defined. There is an open issue about this. > @@ +224,4 @@ > > > > + mNextRedirectCount++; > > + mPreloadEntries.push_back(std::move(entry)); > > + entry.forget(); > > is the forget() needed? doesn't the move ctor move (clears on |entry|) the > pointer? I was not sure. I will check. > > @@ +282,5 @@ > > NS_ENSURE_SUCCESS(rv, rv); > > > > + if (!aRedirectCount) { > > + rv = cacheStorage->OpenTruncate(aUri, > > + nsPrintfCString("preload=u%" PRIu64 "_%" PRIu32, > > why the 'u' before '%'? I do not remember why I added. I can remove it as well. > > @@ +283,5 @@ > > > > + if (!aRedirectCount) { > > + rv = cacheStorage->OpenTruncate(aUri, > > + nsPrintfCString("preload=u%" PRIu64 "_%" PRIu32, > > + aWinId, aRedirectCount), > > aRedirectCount is always 0 yes. > > @@ +289,5 @@ > > + } else { > > + > > + nsAutoCString ex; > > + ex.Append(nsPrintfCString("preload=u%" PRIu64 "_%" PRIu32, aWinId, > > + aRedirectCount)); > > this is identical to the !aRedirectCount case, could we bring it out the > branch? I will. > > why it's not sufficient to just start a new preload for a redirect target > URI? If the page happen to preload a resource that is a redirect target of > a different preload we load twice. Or is this needed to be spec compliant? This is not defined in spec. If a link rel=preload gets removed from the tree, we will need a way to remove all of its redirects. This was easier way to do this. > > @@ +558,3 @@ > > iter2.Data()->CancelPreload(); > > iter2.Remove(); > > + iter2.Next(); > > why calling Next() here? This is a mistake. Thanks. > > ::: uriloader/prefetch/nsPreloadServiceBackend.h > @@ +71,5 @@ > > ~nsPreloadNodeBackend() {} > > > > friend class nsPreloadServiceBackend; > > + nsresult AddLoad(nsIURI *aUri, const nsACString &aSpec, > > + nsILoadContextInfo *aLoadInfo); > > some comments would be nice > I will add them. > @@ +83,2 @@ > > mozilla::TimeStamp mCreated; > > + uint32_t mNextRedirectCount; > > as well here I will add them.

rebased

rebased.

rebased.

rebased.

rebased.

Comment on attachment 8923324 [details] [diff] [review] bug_preload_nocache_v2.patch This was only rebased.

There are a bunch of r+'d patches in this bug. Is anything blocking these from landing?

(In reply to Jon Coppeard (:jonco) (PTO until 24th July) from comment #64) > There are a bunch of r+'d patches in this bug. Is anything blocking these > from landing? We have decided to go with a different approach for rel=preload. I do not know when it will be implemented.