Closed Bug 1447341 Opened 6 years ago Closed 6 years ago

We're excessively accessing unused network.protocol-handler.external. prefs

Categories

(Core :: Networking, enhancement, P3)

Product:
Core
Component:
Networking
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: johannh, Assigned: Gijs)

References

Details

(Whiteboard: [necko-triaged])

Attachments

(5 files)

Bug 1447341 - make blob: protocol handler support nsISupportsWeakReference,
59 bytes, text/x-review-board-request
baku
: review+
Details
Bug 1447341 - don't try to get a protocol handler for empty schemes,
59 bytes, text/x-review-board-request
valentin
: review+
Details
Bug 1447341 - make page-icon protocol handler support weak referencing,
59 bytes, text/x-review-board-request
mak
: review+
Details
Bug 1447341 - include blob, moz-extension and page-icon in list of known schemes,
59 bytes, text/x-review-board-request
u408661
: review+
Details
Bug 1447341 - stop querying prefs for fake protocols we use internally,
59 bytes, text/x-review-board-request
u408661
: review+
Details 
I noticed this while working on bug 1425613. The UsesExternalProtocolHandler (a helper function for nsIOService::GetProtocolHandler) does a pref access for about every single scheme you throw at it, even when there's no way that scheme has a protocol handler applied to it (like network.protocol-handler.external.moz-extension).

https://searchfox.org/mozilla-central/rev/877c99c523a054419ec964d4dfb3f0cadac9d497/netwerk/base/nsIOService.cpp#538

This causes a lot of excess pref access through e.g. Services.io.newURI, for example (some printf debugging results):

- Loading about:newtab is accessing "network.protocol-handler.external.blob" > 20 times.

- Focusing the window is accessing "network.protocol-handler.external.page-icon" ~3 times.

- Every single keystroke in the urlbar is causing one access of "network.protocol-handler.external." (yes, without a scheme) with a ton of extra calls to "network.protocol-handler.external.page-icon" when there are results in the awesomebar.

- Starting the browser causes "network.protocol-handler.external.moz-extension" to be accessed a ton of times.

- Strangely, opening any popup accesses "network.protocol-handler.external.moz-extension" a bunch of times, even on a fresh profile

Again, none of these pref getters is actually useful. All of them will fall back to the default value after doing a hash table lookup. It's not as bad as a pref lookup from JS, but it's still completely unnecessary.

It feels like some of the callers have underlying performance issues that need to be looked into as well if they're doing this so excessively, but optimizing UsesExternalProtocolHandler to avoid calling these prefs sounds like good first low-hanging fruit to me.
There's a caching mechanism on the IO service. Why is that not working?
Flags: needinfo?(jhofmann)
(In reply to :Gijs from comment #1)
> There's a caching mechanism on the IO service. Why is that not working?

Oh, looks like for it to work the protocol handler has to support weak referencing. So we could avoid this by making the blob, page-icon, and moz-extension protocol handler implement weak referencing. With the demise of xpcom add-ons, perhaps we can enforce that internal protocol handlers implement nsISupportsWeakReference, and MOZ_CRASH otherwise.
Looks like we should contemplate this at some point in the future.
Flags: needinfo?(jhofmann)
Priority: -- → P3
Whiteboard: [necko-triaged]
Attachment #8968557 - Flags: review?(hurley)
Attachment #8968558 - Flags: review?(hurley)
Comment on attachment 8968554 [details]
Bug 1447341 - make blob: protocol handler support nsISupportsWeakReference,

https://reviewboard.mozilla.org/r/237232/#review243060
Attachment #8968554 - Flags: review?(amarchesini) → review+
Comment on attachment 8968557 [details]
Bug 1447341 - include blob, moz-extension and page-icon in list of known schemes,

https://reviewboard.mozilla.org/r/237238/#review243136
Attachment #8968557 - Flags: review?(hurley) → review+
Comment on attachment 8968558 [details]
Bug 1447341 - stop querying prefs for fake protocols we use internally,

https://reviewboard.mozilla.org/r/237240/#review243138

::: netwerk/base/nsIOService.h:38
(Diff revision 1)
>  
>  static const char gScheme[][sizeof("moz-safe-about")] =
>      {"chrome", "file", "http", "https", "jar", "data", "about", "moz-safe-about", "resource",
>       "moz-extension", "page-icon", "blob"};
>  
> +static const char gDefaultSchemes[][sizeof("moz-nullprincipal")] =

|gDefaultSchemes| doesn't seem particularly descriptive, to me. Perhaps something like |gForcedExteralSchemes| would make better sense here?
Attachment #8968558 - Flags: review?(hurley) → review+
Comment on attachment 8968556 [details]
Bug 1447341 - make page-icon protocol handler support weak referencing,

https://reviewboard.mozilla.org/r/237236/#review243284
Attachment #8968556 - Flags: review?(mak77) → review+
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Comment on attachment 8968555 [details]
Bug 1447341 - don't try to get a protocol handler for empty schemes,

https://reviewboard.mozilla.org/r/237234/#review243296
Attachment #8968555 - Flags: review?(valentin.gosu) → review+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1299c9a06811
make blob: protocol handler support nsISupportsWeakReference, r=baku
https://hg.mozilla.org/integration/autoland/rev/d4e360e27b2f
don't try to get a protocol handler for empty schemes, r=valentin
https://hg.mozilla.org/integration/autoland/rev/388d0887f62b
make page-icon protocol handler support weak referencing, r=mak
https://hg.mozilla.org/integration/autoland/rev/8b951bb04521
include blob, moz-extension and page-icon in list of known schemes, r=nwgh
https://hg.mozilla.org/integration/autoland/rev/5d965c4d39f5
stop querying prefs for fake protocols we use internally, r=nwgh
Comment on attachment 8968558 [details]
Bug 1447341 - stop querying prefs for fake protocols we use internally,

https://reviewboard.mozilla.org/r/237240/#review243314


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: netwerk/base/nsIOService.cpp:525
(Diff revision 2)
>          // nsExternalProtocolHandler, since internally we rely on being able to
>          // use and read from these URIs.
>          return false;
>      }
>  
> +    for (unsigned int i = 0; i < NS_N(gForcedExternalSchemes); i++) {

Warning: Use range-based for loop instead [clang-tidy: modernize-loop-convert]

 0:06.74     for (unsigned int i = 0; i < NS_N(gForcedExternalSchemes); i++) {
 0:06.74     ^   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 0:06.74         (const auto & gForcedExternalScheme : gForcedExternalSchemes)
 0:06.74 Warning: modernize-use-override in /cache/sa-central/netwerk/base/nsIOService.cpp: 'virtual' is redundant since the function is already declared 'override'
(In reply to Code Review Bot [:reviewbot] from comment #21)
> ::: netwerk/base/nsIOService.cpp:525
> > +    for (unsigned int i = 0; i < NS_N(gForcedExternalSchemes); i++) {
> 
> Warning: Use range-based for loop instead [clang-tidy:
> modernize-loop-convert]

Huh. I just cargo-culted this, and it seems the review bot didn't run for the initial set of patches, and it doesn't look like this is getting the patches backed out, so I guess I'll file a follow-up to update some of these loops.
Depends on: 1454956
Thank you Gijs!
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: