Bug 34184

Summary:

Freeze sandbox attributes on creation

Product:

WebKit

Reporter:

Adam Barth <abarth>

Component:

Page Loading

Assignee:

Adam Barth <abarth>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, darin

Priority:

Version:

528+ (Nightly build)

Hardware:

Other

OS:

OS X 10.5

Attachments:

Description	Flags
Patch	none
Patch	none
Patch for landing	none

Adam Barth

Reported 2010-01-26 14:39:01 PST

Freeze sandbox attributes on creation

Attachments
Patch (7.45 KB, patch) 2010-01-26 14:40 PST, Adam Barth	no flags	Details Formatted Diff Diff
Patch (11.66 KB, patch) 2010-02-02 21:58 PST, Adam Barth	no flags	Details Formatted Diff Diff
Patch for landing (4.18 KB, patch) 2010-03-19 09:24 PDT, Adam Barth	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Adam Barth

Comment 1 2010-01-26 14:40:36 PST

Created attachment 47451 [details] Patch

Adam Barth

Comment 2 2010-01-26 14:41:42 PST

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

Adam Barth

Comment 3 2010-02-02 21:58:42 PST

Created attachment 47993 [details] Patch

Adam Barth

Comment 4 2010-02-02 21:59:20 PST

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

Darin Adler

Comment 5 2010-02-03 09:48:55 PST

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

Comment 6 2010-02-03 10:25:07 PST

> 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 7 2010-02-09 12:44:12 PST

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

Comment 8 2010-02-09 12:57:58 PST

(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

Comment 9 2010-02-09 14:39:38 PST

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

Adam Barth

Comment 10 2010-02-10 00:37:10 PST

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

Adam Barth

Comment 11 2010-03-19 09:24:37 PDT

Created attachment 51161 [details] Patch for landing

Adam Barth

Comment 12 2010-03-19 09:25:48 PDT

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

WebKit Commit Bot

Comment 13 2010-03-19 18:39:17 PDT

Comment on attachment 51161 [details] Patch for landing Clearing flags on attachment: 51161 Committed r56290: <http://trac.webkit.org/changeset/56290>

WebKit Commit Bot

Comment 14 2010-03-19 18:39:22 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.