Bug 34184 – Freeze sandbox attributes on creation

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Adam Barth]

Freeze sandbox attributes on creation

[Adam Barth]

Created attachment 47451 [details]
Patch

[Adam Barth]

I thought there was a bug on this already, but I couldn't find it.  Tests coming.

[Adam Barth]

Created attachment 47993 [details]
Patch

[Adam Barth]

The test doesn't provide 100% coverage for this change, but it's better than nothing.

[Darin Adler]

Comment on attachment 47993 [details]
Patch

Is it safe to have sandbox flags in the frame? Shouldn't we go further and remove them entirely?

The changes in FrameLoader to call isSandboxed on the frame's document seem awkward. Wouldn't it be better to have a private isSandboxed function that did the right thing instead?

[Adam Barth]

> Is it safe to have sandbox flags in the frame? Shouldn't we go further and
> remove them entirely?

I couldn't figure out how to read the allow-script bit out of the document yet, which is why there's a FIXME for that case.  If I convert that call site to use the document's flags, we crash in Frame creation (for some reason the document is non-null and doesn't appear to be a fully initialized Document object).  I suspect this is because of a tricky case during frame construction.  My plan is to look at that call site in a second patch.  Once we remove that, we can remove a bunch of code related to the flags on the frame.

> The changes in FrameLoader to call isSandboxed on the frame's document seem
> awkward. Wouldn't it be better to have a private isSandboxed function that did
> the right thing instead?

Sure.

[Adam Barth]

Comment on attachment 47993 [details]
Patch

I think Darin meant to r- this patch.  In any case, I need to address his feedback.

[Darin Adler]

(In reply to comment #7)
> I think Darin meant to r- this patch.  In any case, I need to address his
> feedback.

For the record, I did r+ it on purpose. I reviewed it and also had some questions for followup. I don’t object, though, to you addressing more of those issues before landing something.

[Adam Barth]

Ok.  I'll address the remainder of your feedback and then land the patch.  Thanks.

[Adam Barth]

Committed r54587: <http://trac.webkit.org/changeset/54587>

[Adam Barth]

Created attachment 51161 [details]
Patch for landing

[Adam Barth]

I seem to have forgotten to land the tests from this patch.

[WebKit Commit Bot]

Comment on attachment 51161 [details]
Patch for landing

Clearing flags on attachment: 51161

Committed r56290: <http://trac.webkit.org/changeset/56290>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.