Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2381163003:
Handle cross origin with src attribution for descendants (Closed)
|
Created:
4 years, 2 months ago by panicker Modified:
4 years, 2 months ago Reviewers:
skobes CC:
apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, chromium-reviews, devtools-reviews_chromium.org, igrigorik, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHandle cross origin with src attribution for descendants
BUG=635596
Patch Set 1 #Patch Set 2 : check for document() in addition to DOMWindow #
Total comments: 14
Messages
Total messages: 16 (4 generated)
panicker@chromium.org changed reviewers: + caseq@chromium.org, skobes@chromium.org
An alternative version of handling cross origin with "src" (while we investigate alternatives)
https://codereview.chromium.org/2381163003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp (right): https://codereview.chromium.org/2381163003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:144: return localOwner->getAttribute(HTMLNames::srcAttr); Is the src attribute updated on navigation? I wonder if it would be better to use something like lastCrossOriginFrame->domWindow()->location()->href().
https://codereview.chromium.org/2381163003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp (right): https://codereview.chromium.org/2381163003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:144: return localOwner->getAttribute(HTMLNames::srcAttr); On 2016/09/30 21:26:31, skobes wrote: > Is the src attribute updated on navigation? I wonder if it would be better to > use something like lastCrossOriginFrame->domWindow()->location()->href(). Disregard; from reading the doc I see we avoid location.href on purpose to avoid leaking information. I'd suggest adding a comment explaining this. https://codereview.chromium.org/2381163003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgentTest.cpp (right): https://codereview.chromium.org/2381163003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgentTest.cpp:144: TEST_F(InspectorWebPerfAgentTest, SanitizedLongTaskName_CrossOrigin) As discussed offline I think it should not be too hard to test parent/child frame cases, you can create a child frame with code similar to how DummyPageHolder creates the main frame.
https://codereview.chromium.org/2381163003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp (right): https://codereview.chromium.org/2381163003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:144: return localOwner->getAttribute(HTMLNames::srcAttr); On 2016/09/30 21:45:01, skobes wrote: > On 2016/09/30 21:26:31, skobes wrote: > > Is the src attribute updated on navigation? I wonder if it would be better to > > use something like lastCrossOriginFrame->domWindow()->location()->href(). > > Disregard; from reading the doc I see we avoid location.href on purpose to avoid > leaking information. I'd suggest adding a comment explaining this. Acknowledged. https://codereview.chromium.org/2381163003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgentTest.cpp (right): https://codereview.chromium.org/2381163003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgentTest.cpp:144: TEST_F(InspectorWebPerfAgentTest, SanitizedLongTaskName_CrossOrigin) On 2016/09/30 21:45:01, skobes wrote: > As discussed offline I think it should not be too hard to test parent/child > frame cases, you can create a child frame with code similar to how > DummyPageHolder creates the main frame. I tried this today (again), it adds non-trivial setup code and still doesn't work as expected. See: https://codereview.chromium.org/2385873003 Even with all that code the child iframe still shows up as "about:blank" and falls into the same-origin code path. Let me know if you have specific suggestions. I suspect if I copied all the code in https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La... it would likely work, but it doesn't seem like a unittest at this point. My conclusion is that this is better suited to a layout test which I will add as follow-up.
lgtm https://codereview.chromium.org/2381163003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgentTest.cpp (right): https://codereview.chromium.org/2381163003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgentTest.cpp:144: TEST_F(InspectorWebPerfAgentTest, SanitizedLongTaskName_CrossOrigin) On 2016/09/30 22:57:15, Shubhie wrote: > On 2016/09/30 21:45:01, skobes wrote: > > As discussed offline I think it should not be too hard to test parent/child > > frame cases, you can create a child frame with code similar to how > > DummyPageHolder creates the main frame. > > I tried this today (again), it adds non-trivial setup code and still doesn't > work as expected. > See: https://codereview.chromium.org/2385873003 > Even with all that code the child iframe still shows up as "about:blank" and > falls into the same-origin code path. > Let me know if you have specific suggestions. > > I suspect if I copied all the code in > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La... > > it would likely work, but it doesn't seem like a unittest at this point. > > My conclusion is that this is better suited to a layout test which I will add as > follow-up. > Acknowledged.
pfeldman@chromium.org changed reviewers: + pfeldman@chromium.org
https://codereview.chromium.org/2381163003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp (right): https://codereview.chromium.org/2381163003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:27: static const char* crossOriginAttribution = "cross-origin"; style: here, above and below: static const char[] https://codereview.chromium.org/2381163003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:75: m_frameContextLocations.add(toDocument(context)->location()); todo: in case of theverge, we potentially have 20 frames with analytics enabled. This makes us collect 20 copies of a pointer to the frames. At some point, we would want to remove this duplication. https://codereview.chromium.org/2381163003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:105: performance->addLongTaskTiming(startTime, endTime, sanitizedLongTaskName( security: you are passing top-level frame as an observer here, so you are reporting page perf to its iframes as is.
Responded to some review comments, code updates are coming. https://codereview.chromium.org/2381163003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp (right): https://codereview.chromium.org/2381163003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:75: m_frameContextLocations.add(toDocument(context)->location()); On 2016/09/30 23:38:50, pfeldman wrote: > todo: in case of theverge, we potentially have 20 frames with analytics enabled. > This makes us collect 20 copies of a pointer to the frames. At some point, we > would want to remove this duplication. Agreed. Will add a TODO. Going forward I want to change the structure to one "agent" which delivers notifications to all performance objects, (and sanitizes the detailed attribution etc) https://codereview.chromium.org/2381163003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:105: performance->addLongTaskTiming(startTime, endTime, sanitizedLongTaskName( On 2016/09/30 23:38:50, pfeldman wrote: > security: you are passing top-level frame as an observer here, so you are > reporting page perf to its iframes as is. - This root is the frame owner for the Performance object: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/timing/Pe... So it is the "observer". - Secondly I'm not sure what you mean by "reporting page perf to its iframes as is" If a cross-origin iframe is observing and the culprit is the top level page, then we fall into the if statement on line 147: "if (observerFrame->tree().isDescendantOf(culpritLocation->frame()" and report a canned string.
https://codereview.chromium.org/2381163003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp (right): https://codereview.chromium.org/2381163003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:27: static const char* crossOriginAttribution = "cross-origin"; On 2016/09/30 23:38:50, pfeldman wrote: > style: here, above and below: static const char[] Done. https://codereview.chromium.org/2381163003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:75: m_frameContextLocations.add(toDocument(context)->location()); On 2016/10/01 00:05:21, Shubhie wrote: > On 2016/09/30 23:38:50, pfeldman wrote: > > todo: in case of theverge, we potentially have 20 frames with analytics > enabled. > > This makes us collect 20 copies of a pointer to the frames. At some point, we > > would want to remove this duplication. > > Agreed. Will add a TODO. > Going forward I want to change the structure to one "agent" which delivers > notifications to all performance objects, (and sanitizes the detailed > attribution etc) Added TODO. (It's worth mentioning that the agent is created only when an observer for long task is added. But I want to change the structure for when we show additional attribution, so the storage and work for sanitizing is shared)
https://codereview.chromium.org/2381163003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp (right): https://codereview.chromium.org/2381163003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:105: performance->addLongTaskTiming(startTime, endTime, sanitizedLongTaskName( Ah, you should not use InspectedFrames then - store the frame and use it instead. InspectedFrames are used in combination with local frame roots in the inspector instrumentation since it is per-local root. Your agent is per-frame, so it would simply point to the LocalFrame. Also, we should rename this, at this point it has nothing to do with the inspector agents.
On 2016/10/01 00:27:27, pfeldman wrote: > https://codereview.chromium.org/2381163003/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp (right): > > https://codereview.chromium.org/2381163003/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:105: > performance->addLongTaskTiming(startTime, endTime, sanitizedLongTaskName( > Ah, you should not use InspectedFrames then - store the frame and use it > instead. > > InspectedFrames are used in combination with local frame roots in the inspector > instrumentation since it is per-local root. Your agent is per-frame, so it would > simply point to the LocalFrame. Also, we should rename this, at this point it > has nothing to do with the inspector agents. Ok I will store the frame, and use it in a separate CL (coming next). I think the subsequent refactor to share the Agent (vs. each Performance object having its own) will bring this in line with the other inspector agents (currently it's mostly sharing the probes). NOTE that I am abandoning this CL (exposing src) in favor of exposing DOMWindow pointer based on further discussion with Andrey & Ilya. Thanks all for the review.
On 2016/10/03 21:24:34, Shubhie wrote: > On 2016/10/01 00:27:27, pfeldman wrote: > > > https://codereview.chromium.org/2381163003/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp > (right): > > > > > https://codereview.chromium.org/2381163003/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:105: > > performance->addLongTaskTiming(startTime, endTime, sanitizedLongTaskName( > > Ah, you should not use InspectedFrames then - store the frame and use it > > instead. > > > > InspectedFrames are used in combination with local frame roots in the > inspector > > instrumentation since it is per-local root. Your agent is per-frame, so it > would > > simply point to the LocalFrame. Also, we should rename this, at this point it > > has nothing to do with the inspector agents. > > Ok I will store the frame, and use it in a separate CL (coming next). > I think the subsequent refactor to share the Agent (vs. each Performance object > having its own) will bring this in line with the other inspector agents > (currently it's mostly sharing the probes). I sent CL https://codereview.chromium.org/2390863002/ to point to stored LocalFrame vs. InspectedFrames::root However it is premature to rename it to something "not Agent" in that it needs to be added to instrumentingAgents()->add..Agent to share the necessary probes. > > > NOTE that I am abandoning this CL (exposing src) in favor of exposing DOMWindow > pointer based on further discussion with Andrey & Ilya. > Thanks all for the review.
panicker@chromium.org changed reviewers: - caseq@chromium.org, pfeldman@chromium.org, skobes@chromium.org
Withdrawing this CL
Description was changed from ========== Handle cross origin with src attribution for descendants BUG=635596 ========== to ========== Handle cross origin with src attribution for descendants BUG=635596 ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
