1340333 - stylo: stop doing permission manager lookups for browser frames during stylo traversal

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Bobby Holley (:bholley)]

See bug 1294915 comment 43.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Precompute the browser permission at frame element creation time. v1

MozReview-Commit-ID: Gm0oYaSKpUY

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8838315 [details] [diff] [review]
Precompute the browser permission at frame element creation time. v1

>   nsGenericHTMLFrameElement(already_AddRefed<mozilla::dom::NodeInfo>& aNodeInfo,
>                             mozilla::dom::FromParser aFromParser)
>     : nsGenericHTMLElement(aNodeInfo)
>     , nsBrowserElement()
>     , mNetworkCreated(aFromParser == mozilla::dom::FROM_PARSER_NETWORK)
>     , mIsPrerendered(false)
>     , mBrowserFrameListenersRegistered(false)
>     , mFrameLoaderCreationDisallowed(false)
>+    , mBrowserFrameAllowed(false)
>   {
>+    // Decide at creation time whether to allow browser frames. This is a bit
>+    // sloppy, since the principal of an element may change if it gets adopted
>+    // to another same-origin global, or the permission might be added in the
>+    // interim. But it seems reasonable to require that an origin have the
>+    // browser permission at the time a frame is created in order for that frame
>+    // to be a browser.
>+    nsIPrincipal *principal = NodePrincipal();
Nit, nsIPrincipal* principal = NodePrincipal()
(though, I would just use NodePrincipal() as param)

I'm tiny bit worried about the performance. Normally frames/iframes don't end up calling TestPermissionFromPrincipal, and 
I don't have any idea whether TestPermissionFromPrincipal is fast or slow.
So, please profile iframe creation time a bit and see how much slower it becomes with this new patch.

Assuming there isn't significant performance regression, r+. If there is, we need to figure out something else.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

On option would be to move the permission check to happen ::AfterSetAttr in case nsGkAtoms::mozbrowser is set, and store mBrowserFrameAllowed there. By default mBrowserFrameAllowed would be false.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

I think I'd prefer to have the check in ::AfterSetAttr. Then we wouldn't need to check !GetBoolAttr(nsGkAtoms::mozbrowser in nsGenericHTMLFrameElement::GetReallyIsBrowser.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

But if there isn't performance regression with the patch, it is fine too.

Comment 6

[Bobby Holley (:bholley)]

Attachment: Eagerly compute whether a frame is really a browser. v2

MozReview-Commit-ID: Gm0oYaSKpUY

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

oops, ::mozbrowser, not ::browser

Comment 8

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e503bb78dd7bba40aab846e5847af732cbb1d1fb

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa1e057ae6fd
Eagerly compute whether a frame is really a browser. r=smaug

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fa1e057ae6fd