Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
879423003:
Move Location to DOMWindow (Closed)
|
Created:
5 years, 10 months ago by Nate Chapin Modified:
5 years, 10 months ago CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, blink-reviews-dom_chromium.org, cmumford, dglazkov+blink, dgrogan, eae+blinkwatch, ed+blinkwatch_opera.com, gavinp+loader_chromium.org, gyuyoung.kim_webkit.org, jsbell+idb_chromium.org, kinuko+fileapi, mlamouri+watch-blink_chromium.org, mvanouwerkerk+watch_chromium.org, nhiroki, rwlbuis, timvolodine, tommyw+watchlist_chromium.org, tyoshino+watch_chromium.org, tzik Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionMove Location to DOMWindow
This allows a RemoteFrame to be navigated via the location API.
* Make DOMWindowProperty RemoteFrame-friendly. Add a localFrame() accessor that returns null if the frame is remote, and use that instead of Frame in many callsites.
* Move DOMWindowProperty registration from LocalDOMWindow to DOMWindow
* Move Location from LocalDOMWindow to DOMWindow.
* Add a bunch of ASSERTs to ensure that a Location attached to a RemoteFrame is not used in a way that allows observation of the current URL.
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=189946
Patch Set 1 #Patch Set 2 : #Patch Set 3 : +test #Patch Set 4 : #
Total comments: 10
Patch Set 5 : Remove DOMWindowProperty changes #
Total comments: 15
Patch Set 6 : Address comments #
Total comments: 9
Patch Set 7 : reset during willDetachFrameHost #
Total comments: 7
Patch Set 8 : resetLocation call to Frame::detach #
Total comments: 7
Patch Set 9 : Rebase, #include cleanup #
Messages
Total messages: 33 (4 generated)
japhet@chromium.org changed reviewers: + dcheng@chromium.org
The interesting changes are in Location, DOMWindowProperty, and moving stuff from LocalDOMWindow to DOMWindow. Everything else should be boring and mechanical.
After thinking about this more, I think there's a third option to reduce the amount of code churn: just don't have Location be a DOMWindowProperty. We'll need to duplicate a minor amount of the cleanup code for the willDetachDocumentInFrame and willDestroyDocumentInFrame callbacks, but I think that's a minor cost. WDYT? https://codereview.chromium.org/879423003/diff/60001/Source/core/css/StyleMed... File Source/core/css/StyleMedia.cpp (right): https://codereview.chromium.org/879423003/diff/60001/Source/core/css/StyleMed... Source/core/css/StyleMedia.cpp:44: FrameView* view = localFrame() ? localFrame()->view() : 0; Nit: nullptr, since you're changing this anyway. https://codereview.chromium.org/879423003/diff/60001/Source/core/frame/DOMWin... File Source/core/frame/DOMWindow.h (right): https://codereview.chromium.org/879423003/diff/60001/Source/core/frame/DOMWin... Source/core/frame/DOMWindow.h:51: { +haraken, is there a threshhold that it's preferable to inline trace() methods vs out-of-lining them? https://codereview.chromium.org/879423003/diff/60001/Source/core/frame/DOMWin... Source/core/frame/DOMWindow.h:222: Nit: remove blank line. https://codereview.chromium.org/879423003/diff/60001/Source/core/frame/DOMWin... File Source/core/frame/DOMWindowProperty.cpp (right): https://codereview.chromium.org/879423003/diff/60001/Source/core/frame/DOMWin... Source/core/frame/DOMWindowProperty.cpp:95: if (m_frame && m_frame->isLocalFrame()) I think it's better to unconditionally toLocalFrame() here. https://codereview.chromium.org/879423003/diff/60001/Source/web/tests/WebFram... File Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/879423003/diff/60001/Source/web/tests/WebFram... Source/web/tests/WebFrameTest.cpp:6918: RemoteNavigationClient() { } Nit: I think it's fine to omit the constructor in this case. https://codereview.chromium.org/879423003/diff/60001/Source/web/tests/WebFram... Source/web/tests/WebFrameTest.cpp:6925: const WebURLRequest& lastRequest() { return m_lastRequest; } Nit: const. https://codereview.chromium.org/879423003/diff/60001/Source/web/tests/WebFram... Source/web/tests/WebFrameTest.cpp:6927: WebURLRequest m_lastRequest; Nit: make this private, since we have an accessor.
(actually adding haraken this time)
PTAL https://codereview.chromium.org/879423003/diff/60001/Source/web/tests/WebFram... File Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/879423003/diff/60001/Source/web/tests/WebFram... Source/web/tests/WebFrameTest.cpp:6918: RemoteNavigationClient() { } On 2015/02/02 19:27:52, dcheng wrote: > Nit: I think it's fine to omit the constructor in this case. Done. https://codereview.chromium.org/879423003/diff/60001/Source/web/tests/WebFram... Source/web/tests/WebFrameTest.cpp:6925: const WebURLRequest& lastRequest() { return m_lastRequest; } On 2015/02/02 19:27:52, dcheng wrote: > Nit: const. Done. https://codereview.chromium.org/879423003/diff/60001/Source/web/tests/WebFram... Source/web/tests/WebFrameTest.cpp:6927: WebURLRequest m_lastRequest; On 2015/02/02 19:27:52, dcheng wrote: > Nit: make this private, since we have an accessor. Done.
lgtm modulo question about oilpan https://codereview.chromium.org/879423003/diff/80001/Source/core/frame/Locati... File Source/core/frame/Location.h (right): https://codereview.chromium.org/879423003/diff/80001/Source/core/frame/Locati... Source/core/frame/Location.h:49: // it needs a manual call to reset() from DOMWindow::reset() to ensure it doesn't retain a stale Frame pointer. Would we still need reset() with Oilpan? I guess we wouldn't want a cached JS reference to location to keep Frame alive. But we should get that for free with WeakMember in an Oilpan build, right? haraken@, sigbjornf@: is the usual convention to wrap these sort of things in #if !ENABLE(OILPAN)?
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
https://codereview.chromium.org/879423003/diff/80001/Source/core/frame/Locati... File Source/core/frame/Location.h (right): https://codereview.chromium.org/879423003/diff/80001/Source/core/frame/Locati... Source/core/frame/Location.h:49: // it needs a manual call to reset() from DOMWindow::reset() to ensure it doesn't retain a stale Frame pointer. On 2015/02/02 21:50:36, dcheng wrote: > Would we still need reset() with Oilpan? I guess we wouldn't want a cached JS > reference to location to keep Frame alive. But we should get that for free with > WeakMember in an Oilpan build, right? > > haraken@, sigbjornf@: is the usual convention to wrap these sort of things in > #if !ENABLE(OILPAN)? We normally keep such Oilpan-specific dispose/reset/... smaller methods without wrapping #if-ery around them. WeakMembers would take care of it wrt safety and not prolonging lifetime, but you don't want to expose GC behavior&timing to the outside world, if possible. https://codereview.chromium.org/881683003/ is an example of the difference in behavior that might arise if you do rely on WeakMembers in this area. Without having looked at details here, but for Oilpan, I think you want to keep a WeakMember and try to find an effective place for the reset() call (DOMWindow::reset() isn't called as part of frame destruction/finalization with Oilpan.)
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/879423003/diff/80001/Source/core/frame/DOMWin... File Source/core/frame/DOMWindow.cpp (right): https://codereview.chromium.org/879423003/diff/80001/Source/core/frame/DOMWin... Source/core/frame/DOMWindow.cpp:109: // Location needs to be reset manually because it doesn't inherit from DOMWindowProperty. I'm just curious, but why can't Location inherit from DOMWindowProperty? https://codereview.chromium.org/879423003/diff/80001/Source/core/frame/DOMWin... File Source/core/frame/DOMWindow.h (right): https://codereview.chromium.org/879423003/diff/80001/Source/core/frame/DOMWin... Source/core/frame/DOMWindow.h:51: EventTargetWithInlineData::trace(visitor); Nit: Make the trace call to a super class a tail call. https://codereview.chromium.org/879423003/diff/80001/Source/core/frame/Locati... File Source/core/frame/Location.cpp (right): https://codereview.chromium.org/879423003/diff/80001/Source/core/frame/Locati... Source/core/frame/Location.cpp:57: ASSERT(m_frame && m_frame->isLocalFrame()); This ASSERT would be redundant, since toLocalFrame has the check. https://codereview.chromium.org/879423003/diff/80001/Source/core/frame/Locati... Source/core/frame/Location.cpp:152: ASSERT(m_frame->isLocalFrame()); Ditto. The same comment for other parts in this file. https://codereview.chromium.org/879423003/diff/80001/Source/core/frame/Locati... File Source/core/frame/Location.h (right): https://codereview.chromium.org/879423003/diff/80001/Source/core/frame/Locati... Source/core/frame/Location.h:49: // it needs a manual call to reset() from DOMWindow::reset() to ensure it doesn't retain a stale Frame pointer. On 2015/02/02 22:53:04, sof wrote: > On 2015/02/02 21:50:36, dcheng wrote: > > Would we still need reset() with Oilpan? I guess we wouldn't want a cached JS > > reference to location to keep Frame alive. But we should get that for free > with > > WeakMember in an Oilpan build, right? > > > > haraken@, sigbjornf@: is the usual convention to wrap these sort of things in > > #if !ENABLE(OILPAN)? > > We normally keep such Oilpan-specific dispose/reset/... smaller methods without > wrapping #if-ery around them. > > WeakMembers would take care of it wrt safety and not prolonging lifetime, but > you don't want to expose GC behavior&timing to the outside world, if possible. > https://codereview.chromium.org/881683003/ is an example of the difference in > behavior that might arise if you do rely on WeakMembers in this area. > > Without having looked at details here, but for Oilpan, I think you want to keep > a WeakMember and try to find an effective place for the reset() call > (DOMWindow::reset() isn't called as part of frame destruction/finalization with > Oilpan.) > Right, in order not to make GC's behavior observable, it is important to clear it explicitly. https://codereview.chromium.org/879423003/diff/80001/Source/core/frame/Remote... File Source/core/frame/RemoteFrame.cpp (right): https://codereview.chromium.org/879423003/diff/80001/Source/core/frame/Remote... Source/core/frame/RemoteFrame.cpp:53: if (windowProxy) I don't think windowProxy can be null. Shall we change it to WindowProxy&? You can do it in a follow-up CL.
https://codereview.chromium.org/879423003/diff/80001/Source/core/frame/DOMWin... File Source/core/frame/DOMWindow.cpp (right): https://codereview.chromium.org/879423003/diff/80001/Source/core/frame/DOMWin... Source/core/frame/DOMWindow.cpp:109: // Location needs to be reset manually because it doesn't inherit from DOMWindowProperty. On 2015/02/03 00:56:09, haraken wrote: > > I'm just curious, but why can't Location inherit from DOMWindowProperty? Location was the only DOMWindowProperty that needed to support both LocalFrame and RemoteFrame. All other properties are LocalFrame only. Two other approaches were explored: - Make DOMWindowProperty support a generic Frame. However, all subclasses of DOMWindowProperty only work properly with LocalFrame, so we end up losing a lot of type safety. - Make DOMWindowProperty and LocalDOMWindowProperty. It was also a big cognitive overhead--we will have DOMWindowProperty and LocalDOMWindowProperty, but no RemoteDOMWindowProperty. It didn't seem to gain much in simplicity. In the end, it seemed better to just special-case Location, since it is unique in being accessible cross-origin.
https://codereview.chromium.org/879423003/diff/80001/Source/core/frame/DOMWin... File Source/core/frame/DOMWindow.cpp (right): https://codereview.chromium.org/879423003/diff/80001/Source/core/frame/DOMWin... Source/core/frame/DOMWindow.cpp:109: // Location needs to be reset manually because it doesn't inherit from DOMWindowProperty. On 2015/02/03 08:22:54, dcheng wrote: > On 2015/02/03 00:56:09, haraken wrote: > > > > I'm just curious, but why can't Location inherit from DOMWindowProperty? > > Location was the only DOMWindowProperty that needed to support both LocalFrame > and RemoteFrame. All other properties are LocalFrame only. > > Two other approaches were explored: > - Make DOMWindowProperty support a generic Frame. However, all subclasses of > DOMWindowProperty only work properly with LocalFrame, so we end up losing a lot > of type safety. > - Make DOMWindowProperty and LocalDOMWindowProperty. It was also a big cognitive > overhead--we will have DOMWindowProperty and LocalDOMWindowProperty, but no > RemoteDOMWindowProperty. It didn't seem to gain much in simplicity. > > In the end, it seemed better to just special-case Location, since it is unique > in being accessible cross-origin. Thanks, sounds reasonable to me.
https://codereview.chromium.org/879423003/diff/80001/Source/core/frame/DOMWin... File Source/core/frame/DOMWindow.h (right): https://codereview.chromium.org/879423003/diff/80001/Source/core/frame/DOMWin... Source/core/frame/DOMWindow.h:51: EventTargetWithInlineData::trace(visitor); On 2015/02/03 00:56:09, haraken wrote: > > Nit: Make the trace call to a super class a tail call. Done. https://codereview.chromium.org/879423003/diff/80001/Source/core/frame/Locati... File Source/core/frame/Location.cpp (right): https://codereview.chromium.org/879423003/diff/80001/Source/core/frame/Locati... Source/core/frame/Location.cpp:57: ASSERT(m_frame && m_frame->isLocalFrame()); On 2015/02/03 00:56:09, haraken wrote: > > This ASSERT would be redundant, since toLocalFrame has the check. Done. https://codereview.chromium.org/879423003/diff/80001/Source/core/frame/Locati... Source/core/frame/Location.cpp:152: ASSERT(m_frame->isLocalFrame()); On 2015/02/03 00:56:09, haraken wrote: > > Ditto. The same comment for other parts in this file. Done. https://codereview.chromium.org/879423003/diff/80001/Source/core/frame/Locati... File Source/core/frame/Location.h (right): https://codereview.chromium.org/879423003/diff/80001/Source/core/frame/Locati... Source/core/frame/Location.h:49: // it needs a manual call to reset() from DOMWindow::reset() to ensure it doesn't retain a stale Frame pointer. On 2015/02/03 00:56:09, haraken wrote: > On 2015/02/02 22:53:04, sof wrote: > > On 2015/02/02 21:50:36, dcheng wrote: > > > Would we still need reset() with Oilpan? I guess we wouldn't want a cached > JS > > > reference to location to keep Frame alive. But we should get that for free > > with > > > WeakMember in an Oilpan build, right? > > > > > > haraken@, sigbjornf@: is the usual convention to wrap these sort of things > in > > > #if !ENABLE(OILPAN)? > > > > We normally keep such Oilpan-specific dispose/reset/... smaller methods > without > > wrapping #if-ery around them. > > > > WeakMembers would take care of it wrt safety and not prolonging lifetime, but > > you don't want to expose GC behavior&timing to the outside world, if possible. > > https://codereview.chromium.org/881683003/ is an example of the difference in > > behavior that might arise if you do rely on WeakMembers in this area. > > > > Without having looked at details here, but for Oilpan, I think you want to > keep > > a WeakMember and try to find an effective place for the reset() call > > (DOMWindow::reset() isn't called as part of frame destruction/finalization > with > > Oilpan.) > > > > Right, in order not to make GC's behavior observable, it is important to clear > it explicitly. I added an unconditional reset() to ~DOMWindow. I think this does what is required, but I'm sufficiently uncertain of oilpan's GC behavior to be confident of it. https://codereview.chromium.org/879423003/diff/80001/Source/core/frame/Remote... File Source/core/frame/RemoteFrame.cpp (right): https://codereview.chromium.org/879423003/diff/80001/Source/core/frame/Remote... Source/core/frame/RemoteFrame.cpp:53: if (windowProxy) On 2015/02/03 00:56:09, haraken wrote: > > I don't think windowProxy can be null. Shall we change it to WindowProxy&? You > can do it in a follow-up CL. Yeah, I'd rather not mix that refactor with this CL if that's ok.
https://codereview.chromium.org/879423003/diff/100001/Source/core/frame/DOMWi... File Source/core/frame/DOMWindow.cpp (right): https://codereview.chromium.org/879423003/diff/100001/Source/core/frame/DOMWi... Source/core/frame/DOMWindow.cpp:19: reset(); This is problematic in an Oilpan setting, as destructors are not allowed to touch other heap objects. For LocalDOMWindow, you could trigger the rest via FrameDestructionObserver's willDetachFrameHost(). Perhaps RemoteDOMWindow needs to become a FrameDestructionObserver (of its RemoteFrame)? https://codereview.chromium.org/879423003/diff/100001/Source/core/frame/DOMWi... File Source/core/frame/DOMWindow.h (right): https://codereview.chromium.org/879423003/diff/100001/Source/core/frame/DOMWi... Source/core/frame/DOMWindow.h:49: void trace(Visitor* visitor) override It would be preferable to have this trace() implementation in DOMWindow.cpp now. https://codereview.chromium.org/879423003/diff/100001/Source/core/frame/DOMWi... Source/core/frame/DOMWindow.h:199: virtual void reset(); Does this need to be virtual?
https://codereview.chromium.org/879423003/diff/100001/Source/core/frame/DOMWi... File Source/core/frame/DOMWindow.cpp (right): https://codereview.chromium.org/879423003/diff/100001/Source/core/frame/DOMWi... Source/core/frame/DOMWindow.cpp:19: reset(); On 2015/02/03 21:25:10, sof wrote: > This is problematic in an Oilpan setting, as destructors are not allowed to > touch other heap objects. > > For LocalDOMWindow, you could trigger the rest via FrameDestructionObserver's > willDetachFrameHost(). Perhaps RemoteDOMWindow needs to become a > FrameDestructionObserver (of its RemoteFrame)? s/rest/reset/g
https://codereview.chromium.org/879423003/diff/100001/Source/core/frame/DOMWi... File Source/core/frame/DOMWindow.cpp (right): https://codereview.chromium.org/879423003/diff/100001/Source/core/frame/DOMWi... Source/core/frame/DOMWindow.cpp:19: reset(); On 2015/02/03 21:25:10, sof wrote: > This is problematic in an Oilpan setting, as destructors are not allowed to > touch other heap objects. > > For LocalDOMWindow, you could trigger the rest via FrameDestructionObserver's > willDetachFrameHost(). Perhaps RemoteDOMWindow needs to become a > FrameDestructionObserver (of its RemoteFrame)? Resetting the location in willDetachFrameHost() has observable side effects for Location objects that are being referenced by JS. On trunk, we report the location's url as about:blank if the location object is detached from its frame. With reset in willDetachFrameHost(), we report undefined. FWIW, Firefox returns an empty string in the same situation. https://codereview.chromium.org/879423003/diff/100001/Source/core/frame/DOMWi... File Source/core/frame/DOMWindow.h (right): https://codereview.chromium.org/879423003/diff/100001/Source/core/frame/DOMWi... Source/core/frame/DOMWindow.h:49: void trace(Visitor* visitor) override On 2015/02/03 21:25:10, sof wrote: > It would be preferable to have this trace() implementation in DOMWindow.cpp now. Done. https://codereview.chromium.org/879423003/diff/100001/Source/core/frame/DOMWi... Source/core/frame/DOMWindow.h:199: virtual void reset(); On 2015/02/03 21:25:10, sof wrote: > Does this need to be virtual? Nope. Renamed to resetLocation anyway, to make its intent clearer.
Looks good, about ready. Could you trigger some trybot runs? https://codereview.chromium.org/879423003/diff/100001/Source/core/frame/DOMWi... File Source/core/frame/DOMWindow.cpp (right): https://codereview.chromium.org/879423003/diff/100001/Source/core/frame/DOMWi... Source/core/frame/DOMWindow.cpp:19: reset(); On 2015/02/06 19:39:58, Nate Chapin wrote: > On 2015/02/03 21:25:10, sof wrote: > > This is problematic in an Oilpan setting, as destructors are not allowed to > > touch other heap objects. > > > > For LocalDOMWindow, you could trigger the rest via FrameDestructionObserver's > > willDetachFrameHost(). Perhaps RemoteDOMWindow needs to become a > > FrameDestructionObserver (of its RemoteFrame)? > > Resetting the location in willDetachFrameHost() has observable side effects for > Location objects that are being referenced by JS. On trunk, we report the > location's url as about:blank if the location object is detached from its frame. > With reset in willDetachFrameHost(), we report undefined. > > FWIW, Firefox returns an empty string in the same situation. I see, somewhat similar to changes seen for https://codereview.chromium.org/881683003/ I'm surprised the change here isn't exercised by existing layout tests; or is it & outputs haven't been updated? https://codereview.chromium.org/879423003/diff/120001/Source/core/frame/Locat... File Source/core/frame/Location.h (left): https://codereview.chromium.org/879423003/diff/120001/Source/core/frame/Locat... Source/core/frame/Location.h:48: WILL_BE_USING_GARBAGE_COLLECTED_MIXIN(Location); With Location no longer deriving from DOMWindowProperty, it is no longer a mixin, so this needs to be removed also. https://codereview.chromium.org/879423003/diff/120001/Source/core/frame/Locat... File Source/core/frame/Location.h (right): https://codereview.chromium.org/879423003/diff/120001/Source/core/frame/Locat... Source/core/frame/Location.h:97: RawPtrWillBeWeakMember<Frame> m_frame; Don't need a weak reference here as the reference is cleared on detach in any case; switch to RawPtrWillBeMember<> ?
https://codereview.chromium.org/879423003/diff/120001/Source/core/frame/Locat... File Source/core/frame/Location.h (right): https://codereview.chromium.org/879423003/diff/120001/Source/core/frame/Locat... Source/core/frame/Location.h:97: RawPtrWillBeWeakMember<Frame> m_frame; On 2015/02/07 17:09:32, sof wrote: > Don't need a weak reference here as the reference is cleared on detach in any > case; switch to RawPtrWillBeMember<> ? I'm curious about this. It seems that Location should not keep Frame alive, so this needs to be a WeakMember even though it is explicitly cleared?
https://codereview.chromium.org/879423003/diff/120001/Source/core/frame/Locat... File Source/core/frame/Location.h (right): https://codereview.chromium.org/879423003/diff/120001/Source/core/frame/Locat... Source/core/frame/Location.h:97: RawPtrWillBeWeakMember<Frame> m_frame; On 2015/02/08 01:19:40, haraken wrote: > On 2015/02/07 17:09:32, sof wrote: > > Don't need a weak reference here as the reference is cleared on detach in any > > case; switch to RawPtrWillBeMember<> ? > > I'm curious about this. It seems that Location should not keep Frame alive, so > this needs to be a WeakMember even though it is explicitly cleared? It could only keep it separately alive if the reference to Location from the DOMWindow is independently lost, right? When is the DOMWindow=>Location reference lost? When it is reset, which happens at the same time that this (Location=>DOMWindow) reference is cleared.
On 2015/02/08 06:42:13, sof wrote: > https://codereview.chromium.org/879423003/diff/120001/Source/core/frame/Locat... > File Source/core/frame/Location.h (right): > > https://codereview.chromium.org/879423003/diff/120001/Source/core/frame/Locat... > Source/core/frame/Location.h:97: RawPtrWillBeWeakMember<Frame> m_frame; > On 2015/02/08 01:19:40, haraken wrote: > > On 2015/02/07 17:09:32, sof wrote: > > > Don't need a weak reference here as the reference is cleared on detach in > any > > > case; switch to RawPtrWillBeMember<> ? > > > > I'm curious about this. It seems that Location should not keep Frame alive, so > > this needs to be a WeakMember even though it is explicitly cleared? > > It could only keep it separately alive if the reference to Location from the > DOMWindow is independently lost, right? > > When is the DOMWindow=>Location reference lost? When it is reset, which happens > at the same time that this (Location=>DOMWindow) reference is cleared. I think Member<Frame> is now in order, but should that require adjustments, I will take care of that Oilpan-specific change. Let's get this CL ready & on the way.
https://codereview.chromium.org/879423003/diff/100001/Source/core/frame/DOMWi... File Source/core/frame/DOMWindow.cpp (right): https://codereview.chromium.org/879423003/diff/100001/Source/core/frame/DOMWi... Source/core/frame/DOMWindow.cpp:19: reset(); On 2015/02/07 17:09:32, sof wrote: > On 2015/02/06 19:39:58, Nate Chapin wrote: > > On 2015/02/03 21:25:10, sof wrote: > > > This is problematic in an Oilpan setting, as destructors are not allowed to > > > touch other heap objects. > > > > > > For LocalDOMWindow, you could trigger the rest via > FrameDestructionObserver's > > > willDetachFrameHost(). Perhaps RemoteDOMWindow needs to become a > > > FrameDestructionObserver (of its RemoteFrame)? > > > > Resetting the location in willDetachFrameHost() has observable side effects > for > > Location objects that are being referenced by JS. On trunk, we report the > > location's url as about:blank if the location object is detached from its > frame. > > With reset in willDetachFrameHost(), we report undefined. > > > > FWIW, Firefox returns an empty string in the same situation. > > I see, somewhat similar to changes seen for > https://codereview.chromium.org/881683003/ > > I'm surprised the change here isn't exercised by existing layout tests; or is it > & outputs haven't been updated? There was a single layout test that had output changed. I didn't bother to update the results because I was still debating internally as to whether this was OK, and if it was, whether we should match Firefox's output rather than the current default of undefined output.
It looks like window.location is also still accessible on a detached Window in IE. That being said, I can't really see a use for being able to reference window.location on a detached DOMWindow. However, the behavior of Window properties on a detached frame is not well-defined... perhaps it's something we should ask whatwg@ about. https://codereview.chromium.org/879423003/diff/120001/Source/core/frame/Remot... File Source/core/frame/RemoteFrame.cpp (right): https://codereview.chromium.org/879423003/diff/120001/Source/core/frame/Remot... Source/core/frame/RemoteFrame.cpp:78: m_domWindow->resetLocation(); Hmm... is this something we could get away with doing in Frame::detach? Right now, it looks like we have to call resetLocation() in three separate locations, which doesn't seem ideal.
On 2015/02/09 23:29:58, dcheng wrote: > It looks like window.location is also still accessible on a detached Window in > IE. > > That being said, I can't really see a use for being able to reference > window.location on a detached DOMWindow. However, the behavior of Window > properties on a detached frame is not well-defined... perhaps it's something we > should ask whatwg@ about. > Most likely already known, but the reason why ".location" isn't accessible is the binding security check for Location doesn't permit frameless access, see https://codereview.chromium.org/881683003/#msg8
https://codereview.chromium.org/881683003/ updated the test expectations for the test that was failing with this patch, so this should now have no observable effects on Location lifetime. https://codereview.chromium.org/879423003/diff/120001/Source/core/frame/Locat... File Source/core/frame/Location.h (left): https://codereview.chromium.org/879423003/diff/120001/Source/core/frame/Locat... Source/core/frame/Location.h:48: WILL_BE_USING_GARBAGE_COLLECTED_MIXIN(Location); On 2015/02/07 17:09:32, sof wrote: > With Location no longer deriving from DOMWindowProperty, it is no longer a > mixin, so this needs to be removed also. Done. https://codereview.chromium.org/879423003/diff/120001/Source/core/frame/Remot... File Source/core/frame/RemoteFrame.cpp (right): https://codereview.chromium.org/879423003/diff/120001/Source/core/frame/Remot... Source/core/frame/RemoteFrame.cpp:78: m_domWindow->resetLocation(); On 2015/02/09 23:29:58, dcheng wrote: > Hmm... is this something we could get away with doing in Frame::detach? > > Right now, it looks like we have to call resetLocation() in three separate > locations, which doesn't seem ideal. Done.
https://codereview.chromium.org/879423003/diff/140001/Source/core/frame/Local... File Source/core/frame/LocalDOMWindow.cpp (right): https://codereview.chromium.org/879423003/diff/140001/Source/core/frame/Local... Source/core/frame/LocalDOMWindow.cpp:652: resetLocation(); Is it still necessary to call this here? Is this because someone can try accessing window.location on a detached window, causing this to be recreated? https://codereview.chromium.org/879423003/diff/140001/Source/core/frame/Remot... File Source/core/frame/RemoteDOMWindow.cpp (right): https://codereview.chromium.org/879423003/diff/140001/Source/core/frame/Remot... Source/core/frame/RemoteDOMWindow.cpp:12: #include "core/frame/Location.h" It's not clear to me why we need this include. https://codereview.chromium.org/879423003/diff/140001/Source/core/frame/Remot... File Source/core/frame/RemoteFrame.cpp (right): https://codereview.chromium.org/879423003/diff/140001/Source/core/frame/Remot... Source/core/frame/RemoteFrame.cpp:52: WindowProxy* windowProxy = m_windowProxyManager->windowProxy(world); FWIW, I landed a small patch to fix this outside of this patch, so you might need to rebase.
lgtm https://codereview.chromium.org/879423003/diff/140001/Source/core/frame/Remot... File Source/core/frame/RemoteFrame.cpp (right): https://codereview.chromium.org/879423003/diff/140001/Source/core/frame/Remot... Source/core/frame/RemoteFrame.cpp:52: WindowProxy* windowProxy = m_windowProxyManager->windowProxy(world); On 2015/02/10 23:02:18, dcheng wrote: > FWIW, I landed a small patch to fix this outside of this patch, so you might > need to rebase. Done.
On 2015/02/10 23:22:02, Nate Chapin wrote: > lgtm > > https://codereview.chromium.org/879423003/diff/140001/Source/core/frame/Remot... > File Source/core/frame/RemoteFrame.cpp (right): > > https://codereview.chromium.org/879423003/diff/140001/Source/core/frame/Remot... > Source/core/frame/RemoteFrame.cpp:52: WindowProxy* windowProxy = > m_windowProxyManager->windowProxy(world); > On 2015/02/10 23:02:18, dcheng wrote: > > FWIW, I landed a small patch to fix this outside of this patch, so you might > > need to rebase. > > Done. In other news, I really should look which window I'm in before trying to approve a patch.
https://codereview.chromium.org/879423003/diff/140001/Source/core/frame/Local... File Source/core/frame/LocalDOMWindow.cpp (right): https://codereview.chromium.org/879423003/diff/140001/Source/core/frame/Local... Source/core/frame/LocalDOMWindow.cpp:652: resetLocation(); On 2015/02/10 23:02:18, dcheng wrote: > Is it still necessary to call this here? Is this because someone can try > accessing window.location on a detached window, causing this to be recreated? This is also called at commit time. Documents shouldn't be sharing the same location object, right? https://codereview.chromium.org/879423003/diff/140001/Source/core/frame/Remot... File Source/core/frame/RemoteDOMWindow.cpp (right): https://codereview.chromium.org/879423003/diff/140001/Source/core/frame/Remot... Source/core/frame/RemoteDOMWindow.cpp:12: #include "core/frame/Location.h" On 2015/02/10 23:02:18, dcheng wrote: > It's not clear to me why we need this include. Because I had omitted it in DOMWindow.h, will add there.
LGTM
lgtm https://codereview.chromium.org/879423003/diff/140001/Source/core/frame/Local... File Source/core/frame/LocalDOMWindow.cpp (right): https://codereview.chromium.org/879423003/diff/140001/Source/core/frame/Local... Source/core/frame/LocalDOMWindow.cpp:652: resetLocation(); On 2015/02/10 23:30:18, Nate Chapin wrote: > On 2015/02/10 23:02:18, dcheng wrote: > > Is it still necessary to call this here? Is this because someone can try > > accessing window.location on a detached window, causing this to be recreated? > > This is also called at commit time. Documents shouldn't be sharing the same > location object, right? IIRC, this is called at commit-time on the old DOMWindow. So I guess the question is if the old Location object should still be able to navigate the current frame. I guess the answer is no, but this is certainly an area of the code that could use some cleanup (I guess this kind of goes with my long-term plans to proactively null out frame on DOMWindow).
The CQ bit was checked by japhet@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/879423003/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://src.chromium.org/viewvc/blink?view=rev&revision=189946 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
