1539906 - Is it expected that preconnect (speculative connect) machinery can synchronously run script?

Status: NEW

(Core :: Networking, enhancement, P3)

Description

[Boris Zbarsky [:bzbarsky]]

I was trying to annotate nsIObserverService::NotifyObservers() as "can run script", and it turns out that preconnect machinery can synchronously call into that, via PSM bits that notify about cert when DNS lookups happen, etc, etc. The predictor stuff is involved as well.

Is that expected? Should all this be done async somehow? We currently kick off preconnects from all sorts of places in the DOM, and while I could add async behavior at all of those it seems like it might be better to centralize the async parts.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Alternately, maybe PSM should either not send the notifications sync or not use a general observer service notification for things that are meant for one single consumer (e.g. I am looking at "psm:user-certificate-added", "psm:user-certificate-deleted", "net:cancel-all-connections").

It might also be good to think about an alternate implementation for some things that are test-only but end up infecting a bunch of the codebase with can-run-script (e.g. the "data-storage-ready" and "data-storage-written" notifications).

Comment 2

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

I have no objections to improving how this works.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Well, ok, but what does "improving" mean in this case? Can PSM switch to notifying async, or would that break something? Who owns this general area of code?

Just to be clear, I don't have time right now to change around the PSM stuff; it's just blocking safety improvements in other parts of Gecko at the moment... We could probably MOZ_CAN_RUN_SCRIPT_BOUNDARY on the PSM notifications and move on to unblock the work in bug 1539808, but it would be good to have a long-term plan.

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

I had a quick look and I believe "psm:user-certificate-added", "psm:user-certificate-deleted", "data-storage-ready", and "data-storage-written" can all be made asynchronous without breaking anything, if that's the quickest way to get you unblocked. Making "net:cancel-all-connections" asynchronous might cause intermittent failures in security/manager/ssl/tests/mochitest/browser/browser_clientAuth_connection.js if we can't find an alternative way to have the test wait until necko has actually terminated all connections, but that's the only potential problem I saw.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Thank you for checking that! I'll take a look at doing that and see where that lands me.

Comment 6

[Dragana Damjanovic [:dragana]]

I do not have any objectives, anyway keeleris better person to judge about PSM.
About "net:cancel-all-connections" we can look how we can achieve the same behavior in a different way. This is only for tests.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Mark DOM bits that end up calling into necko preconnect machinery as MOZ_CAN_RUN_SCRIPT.

I've had this in my tree for a bit. Attaching here so it does not get lost.