Bug 34184

Summary: Freeze sandbox attributes on creation
Product: WebKit Reporter: Adam Barth <abarth>
Component: Page LoadingAssignee: Adam Barth <abarth>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing none

Description Adam Barth 2010-01-26 14:39:01 PST
Freeze sandbox attributes on creation
Comment 1 Adam Barth 2010-01-26 14:40:36 PST
Created attachment 47451 [details]
Patch
Comment 2 Adam Barth 2010-01-26 14:41:42 PST
I thought there was a bug on this already, but I couldn't find it.  Tests coming.
Comment 3 Adam Barth 2010-02-02 21:58:42 PST
Created attachment 47993 [details]
Patch
Comment 4 Adam Barth 2010-02-02 21:59:20 PST
The test doesn't provide 100% coverage for this change, but it's better than nothing.
Comment 5 Darin Adler 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?
Comment 6 Adam Barth 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.
Comment 7 Adam Barth 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.
Comment 8 Darin Adler 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.
Comment 9 Adam Barth 2010-02-09 14:39:38 PST
Ok.  I'll address the remainder of your feedback and then land the patch.  Thanks.
Comment 10 Adam Barth 2010-02-10 00:37:10 PST
Committed r54587: <http://trac.webkit.org/changeset/54587>
Comment 11 Adam Barth 2010-03-19 09:24:37 PDT
Created attachment 51161 [details]
Patch for landing
Comment 12 Adam Barth 2010-03-19 09:25:48 PDT
I seem to have forgotten to land the tests from this patch.
Comment 13 WebKit Commit Bot 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>
Comment 14 WebKit Commit Bot 2010-03-19 18:39:22 PDT
All reviewed patches have been landed.  Closing bug.