1968390 - If nsPrefetchService is not initialized before document load, <link rel=prefetch> might never be fetched.

Status: RESOLVED FIXED

(Core :: Networking, defect, P2)

Description

[Emilio Cobos Álvarez [:emilio]]

Bug 1968202 was backed out (in bug 1968202 comment 5) because of a failure in prefetch-transfer-size-executor.html.

That test basically loads a page and adds a <link rel=prefetch as=document>, waiting for its load event.

After my patch, the load event didn't arrive. I debugged it a little bit and it turns out that before my patch the prefetch service gets initialized before that page load (due to the built-in accessiblecaret stylesheet).

That means that nsPrefetchService gets initialized, watches the document load, and sets mHaveProcessed = true, which triggers the prefetch here.

If that load doesn't trigger, nsPrefetchService gets initialized lazily after the doc has loaded, so mHaveProcessed is false and never trigger the load (unless some other document loads in that process).

It is extremely weird to have a global service depend on whether any document has loaded to start working... For now I'm preserving the current behavior of initializing the prefetch service, but this should be fixed.

mHaveProcessed comes from bug 372970. It seems a low risk fix may be that mHaveProcessed should be initialized to "is there any ongoing document load"? But also the utility of mHaveProcessed seems very narrow, so maybe the better fix is just removing that member.

Valentin, do you know who may be familiar with the prefetch service? I'm happy to implement either approach but if someone is more familiar with this code than me it'd be appreciated. FWIW mHaveProcessed seems pretty useless to me, it may only be useful during the first load of a document in a given process... which is a weird thing to watch for? :)

Comment 1

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1968390 - Minor clean-ups to nsPrefetchService. r=#necko-reviewers

No behavior change. Remove an unused method that does nothing too.

Comment 2

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1968390 - Remove nsPrefetchService::mHaveProcessed. r=#necko-reviewers

It's not correct, see comment 0. This in theory can get preload / prefetch
started a bit sooner on the first pageload of a document. But that might
actually be fine? It's the current behavior too if you init the service and an
iframe loads.

Comment 3

[Valentin Gosu [:valentin]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #0)

Valentin, do you know who may be familiar with the prefetch service? I'm happy to implement either approach but if someone is more familiar with this code than me it'd be appreciated. FWIW mHaveProcessed seems pretty useless to me, it may only be useful during the first load of a document in a given process... which is a weird thing to watch for? :)

I think Dragana was the last one to do any meaningful work on the prefetch service. Since then a few members of the Necko team have fixed minor bugs, but nothing substantial. So I expect your grasp of the code is similar or better than anyone's 🙂

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5facaf729bb1
Minor clean-ups to nsPrefetchService. r=necko-reviewers,valentin
https://hg.mozilla.org/integration/autoland/rev/03159fa68b2b
Remove nsPrefetchService::mHaveProcessed. r=necko-reviewers,valentin

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/733f4f35a1d9
Revert "Bug 1968390 - Remove nsPrefetchService::mHaveProcessed. r=necko-reviewers,valentin" for causing xpcshell failures in test_cancelPrefetch.js.

Comment 6

[Serban Stanca [:SerbanS]]

Backed out for causing xpcshell failures in test_cancelPrefetch.js.

• Backout link
• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-FAIL | dom/base/test/unit/test_cancelPrefetch.js | xpcshell return code: 0

Comment 7

[Emilio Cobos Álvarez [:emilio]]

So this test fails because the prefetch is blocked (since the doc is a DOMParser created document and CSP blocks all loads), and now we try to start the prefetch immediately since there's no document running.

Valentin, what should I do about this? It seems this functionality is already covered by test_link_prefetch.html (this call in particular).

Comment 8

[Valentin Gosu [:valentin]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #7)

Valentin, what should I do about this? It seems this functionality is already covered by test_link_prefetch.html (this call in particular).

Thanks, that test seems to cover everything in test_cancelPrefetch.js - so I'm OK with removing the unit test.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c3085b787071
Minor clean-ups to nsPrefetchService. r=necko-reviewers,valentin
https://hg.mozilla.org/integration/autoland/rev/58a442a2c579
Remove nsPrefetchService::mHaveProcessed. r=necko-reviewers,valentin

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3674d8e9144b
Remove another unit test for a behavior that's covered by mochitest / wpt.

Comment 11

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/c3085b787071
https://hg.mozilla.org/mozilla-central/rev/58a442a2c579
https://hg.mozilla.org/mozilla-central/rev/3674d8e9144b