Closed Bug 1340333 Opened 3 years ago Closed 3 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(1 file, 1 obsolete file)

MozReview-Commit-ID: Gm0oYaSKpUY
Attachment #8838315 - Flags: review?(bugs)
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.
Attachment #8838315 - Flags: review?(bugs) → review+
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.
I think I'd prefer to have the check in ::AfterSetAttr. Then we wouldn't need to check !GetBoolAttr(nsGkAtoms::mozbrowser in nsGenericHTMLFrameElement::GetReallyIsBrowser.
But if there isn't performance regression with the patch, it is fine too.
MozReview-Commit-ID: Gm0oYaSKpUY
Attachment #8838332 - Flags: review?(bugs)
Attachment #8838315 - Attachment is obsolete: true
Assignee: nobody → bobbyholley
Attachment #8838332 - Flags: review?(bugs) → review+
oops, ::mozbrowser, not ::browser
Pushed by bholley@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa1e057ae6fd
Eagerly compute whether a frame is really a browser. r=smaug
https://hg.mozilla.org/mozilla-central/rev/fa1e057ae6fd
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.