Bug 14319 - Move the Selection object out of the JS bindings
Summary: Move the Selection object out of the JS bindings
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 523.x (Safari 3)
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Nobody
Depends on:
Blocks: 14192
  Show dependency treegraph
Reported: 2007-06-22 15:47 PDT by Sam Weinig
Modified: 2007-06-27 22:27 PDT (History)
0 users

See Also:

patch (42.41 KB, patch)
2007-06-22 15:56 PDT, Sam Weinig
ggaren: review-
Details | Formatted Diff | Diff
updated patch (43.62 KB, patch)
2007-06-24 18:19 PDT, Sam Weinig
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2007-06-22 15:47:44 PDT
Forthcoming patch moves the Selection object out of the JS binding.  I have renamed the class DOMSelection to avoid a name clash, but this is not necessarily a long term solution.
Comment 1 Sam Weinig 2007-06-22 15:56:31 PDT
Created attachment 15189 [details]
Comment 2 Geoffrey Garen 2007-06-22 16:25:03 PDT
Assuming disconnectFrame() gets called somewhere, DOMSelection needs to NULL check m_frame in each of its member functions.

+void DOMSelection::disconnectFrame()

I don't see any callers to this function. That means that, once the frame is destroyed, referencing its DOMSelection will read/write freed memory, right?

I believe this used to be called by Window::disconnectFrame(). Who should call it now?

+        mutable RefPtr<DOMSelection> m_selection;

Should probably call this m_DOMSelection to distinguish form WebCore::Selection.

There seems to be a cross-frame security problem here. Once I have a window's DOMSelection object, I can read the current selection in the window even if a new document from a new domain is loaded. I can't tell if your patch introduced this bug or not, but it's pretty serious.

r- for everything but the rename.
Comment 3 Sam Weinig 2007-06-24 18:19:45 PDT
Created attachment 15210 [details]
updated patch
Comment 4 Geoffrey Garen 2007-06-26 14:26:21 PDT
Comment on attachment 15210 [details]
updated patch

+        return "";

A different way to do this is to return String(), which gives you the null string. I'm not sure which we want here -- null string or empty string -- though. Empty seems fine.

This patch doesn't fix the security problem, but, like I said, I don't think it introduced the problem, either. Can you file a new bug about it?

One solution to the security problem would be to call DOMSelection::disconnectFrame inside Window::clear. Another solution would be to include isSafeScript checks in the JSSelection object.
Comment 5 Sam Weinig 2007-06-27 22:27:54 PDT
Landed in r23842.  I will open another bug for issue Geoff pointed out.