Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2774543003:
Fix PerformanceNavigationTiming accessor behavior after document detach. (Closed)
DescriptionFix PerformanceNavigationTiming accessor behavior after document detach.
Currently, PerformanceNavigationTiming holds on to a LocalFrame directly.
A DocumentLoader will be extracted at runtime from this LocalFrame. During the
lifetime of a LocalFrame, multiple documents can get loaded and get attached
to the same frame, which causes old PNT instance to reference DocumentLoader
that could be created for new cross-origin document. Therefore, instead of
holding on to a LocalFrame, PNT should hold on to a Document instead.
The change in this patch also fixes the crash reported by clusterfuzz, which
is dereferencing a null pointer when PerformanceNavigationTiming::type gets called
after a Document gets replaced which causes its associated DocumentLoader to be null.
BUG=704352, 703540
Review-Url: https://codereview.chromium.org/2774543003
Cr-Commit-Position: refs/heads/master@{#460198}
Committed: https://chromium.googlesource.com/chromium/src/+/47b93d128610246960c032a00b67ec2083b2a05b
Patch Set 1 : first patch #Patch Set 2 : add layout test #
Total comments: 6
Patch Set 3 : addressed comments #
Total comments: 2
Patch Set 4 : sync #
Messages
Total messages: 34 (23 generated)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by sunjian@chromium.org
The CQ bit was unchecked by sunjian@chromium.org
The CQ bit was checked by sunjian@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by sunjian@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #2 (id:50001) has been deleted
Description was changed from ========== Fix cross-origin security issue raised by PerformanceNavigationTiming. Currently, PerformanceNavigationTiming holds on to a LocalFrame directly. A DocumentLoader will be extracted at runtime from this LocalFrame. During the lifetime of a LocalFrame, multiple documents can get loaded and get attached to the same frame, which causes old PNT instance to reference DocumentLoader that could be created for new cross-origin document. Therefore, instead of holding on to a LocalFrame, PNT should hold on to a Document instead. BUG=704352 ========== to ========== Fix cross-origin security issue raised by PerformanceNavigationTiming. Currently, PerformanceNavigationTiming holds on to a LocalFrame directly. A DocumentLoader will be extracted at runtime from this LocalFrame. During the lifetime of a LocalFrame, multiple documents can get loaded and get attached to the same frame, which causes old PNT instance to reference DocumentLoader that could be created for new cross-origin document. Therefore, instead of holding on to a LocalFrame, PNT should hold on to a Document instead. BUG=704352, 703540 ==========
Description was changed from ========== Fix cross-origin security issue raised by PerformanceNavigationTiming. Currently, PerformanceNavigationTiming holds on to a LocalFrame directly. A DocumentLoader will be extracted at runtime from this LocalFrame. During the lifetime of a LocalFrame, multiple documents can get loaded and get attached to the same frame, which causes old PNT instance to reference DocumentLoader that could be created for new cross-origin document. Therefore, instead of holding on to a LocalFrame, PNT should hold on to a Document instead. BUG=704352, 703540 ========== to ========== Fix cross-origin security issue raised by PerformanceNavigationTiming. Currently, PerformanceNavigationTiming holds on to a LocalFrame directly. A DocumentLoader will be extracted at runtime from this LocalFrame. During the lifetime of a LocalFrame, multiple documents can get loaded and get attached to the same frame, which causes old PNT instance to reference DocumentLoader that could be created for new cross-origin document. Therefore, instead of holding on to a LocalFrame, PNT should hold on to a Document instead. The change in this patch also fixes the crash reported by clusterfuzz, which is dereferencing a null pointer when PerformanceNavigationTiming::type gets called after a Document gets replaced which causes its associated DocumentLoader to be null. BUG=704352, 703540 ==========
sunjian@chromium.org changed reviewers: + japhet@chromium.org, panicker@chromium.org
Please take a look!
Behavior changes is fine, just a few test nitpicks. https://codereview.chromium.org/2774543003/diff/70001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/external/wpt/navigation-timing/nav2_test_document_replaced.html (right): https://codereview.chromium.org/2774543003/diff/70001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/navigation-timing/nav2_test_document_replaced.html:18: pnt1 = navigation_frame.performance.getEntriesByType("navigation")[0]; Nit: move this to the case 1 in the switch statement? https://codereview.chromium.org/2774543003/diff/70001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/navigation-timing/nav2_test_document_replaced.html:20: setTimeout("nav_frame();", 100); Is a 100ms setTimeout() necessary? At least in chromium, you'll need a setTimeout(, 0) to ensure a back/forward entry is created, but 100ms x3 is a lot of stalling. https://codereview.chromium.org/2774543003/diff/70001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/navigation-timing/nav2_test_document_replaced.html:57: This test validates two different PerformanceNavigationTiming instances don't reference the same DocumentLoader when computing their This test will get upstreamed to wpt, right? You probably shouldn't reference a specific class. Maybe: This test validates that a PerformanceNavigatingTiming corresponding to a detached document can't access a different document's state.
Comments addressed. https://codereview.chromium.org/2774543003/diff/70001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/external/wpt/navigation-timing/nav2_test_document_replaced.html (right): https://codereview.chromium.org/2774543003/diff/70001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/navigation-timing/nav2_test_document_replaced.html:18: pnt1 = navigation_frame.performance.getEntriesByType("navigation")[0]; On 2017/03/27 21:44:35, Nate Chapin wrote: > Nit: move this to the case 1 in the switch statement? Done. https://codereview.chromium.org/2774543003/diff/70001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/navigation-timing/nav2_test_document_replaced.html:20: setTimeout("nav_frame();", 100); On 2017/03/27 21:44:35, Nate Chapin wrote: > Is a 100ms setTimeout() necessary? At least in chromium, you'll need a > setTimeout(, 0) to ensure a back/forward entry is created, but 100ms x3 is a lot > of stalling. Acknowledged. https://codereview.chromium.org/2774543003/diff/70001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/navigation-timing/nav2_test_document_replaced.html:57: This test validates two different PerformanceNavigationTiming instances don't reference the same DocumentLoader when computing their On 2017/03/27 21:44:35, Nate Chapin wrote: > This test will get upstreamed to wpt, right? You probably shouldn't reference a > specific class. Maybe: > > This test validates that a PerformanceNavigatingTiming corresponding to a > detached document can't access a different document's state. Acknowledged.
lgtm https://codereview.chromium.org/2774543003/diff/90001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/external/wpt/navigation-timing/nav2_test_frame_removed.html (right): https://codereview.chromium.org/2774543003/diff/90001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/navigation-timing/nav2_test_frame_removed.html:15: assert_equals(pnt.type, "navigate"); By the way, is this behavior actually in the spec? If not, should it be?
On 2017/03/27 23:52:20, Nate Chapin wrote: > lgtm > > https://codereview.chromium.org/2774543003/diff/90001/third_party/WebKit/Layo... > File > third_party/WebKit/LayoutTests/external/wpt/navigation-timing/nav2_test_frame_removed.html > (right): > > https://codereview.chromium.org/2774543003/diff/90001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/external/wpt/navigation-timing/nav2_test_frame_removed.html:15: > assert_equals(pnt.type, "navigate"); > By the way, is this behavior actually in the spec? If not, should it be? +panicker, Hey shubhie, maybe we should mentioned this to the spec people. It seems this should be a reasonable assumption. What do you think?
LGTM https://codereview.chromium.org/2774543003/diff/90001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/external/wpt/navigation-timing/nav2_test_frame_removed.html (right): https://codereview.chromium.org/2774543003/diff/90001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/navigation-timing/nav2_test_frame_removed.html:15: assert_equals(pnt.type, "navigate"); On 2017/03/27 23:52:20, Nate Chapin wrote: > By the way, is this behavior actually in the spec? If not, should it be? This seems like the right default based on description of "navigate" here: https://www.w3.org/TR/navigation-timing-2/#dom-performancenavigationtiming-type "Navigation started by clicking on a link, or entering the URL in the user agent's address bar, or form submission, or initializing through a script operation other than the ones used by reload and back_forward as listed below." Let me know if you think it needs further clarification.
Description was changed from ========== Fix cross-origin security issue raised by PerformanceNavigationTiming. Currently, PerformanceNavigationTiming holds on to a LocalFrame directly. A DocumentLoader will be extracted at runtime from this LocalFrame. During the lifetime of a LocalFrame, multiple documents can get loaded and get attached to the same frame, which causes old PNT instance to reference DocumentLoader that could be created for new cross-origin document. Therefore, instead of holding on to a LocalFrame, PNT should hold on to a Document instead. The change in this patch also fixes the crash reported by clusterfuzz, which is dereferencing a null pointer when PerformanceNavigationTiming::type gets called after a Document gets replaced which causes its associated DocumentLoader to be null. BUG=704352, 703540 ========== to ========== Fix PerformanceNavigationTiming accessor behavior after document detach. Currently, PerformanceNavigationTiming holds on to a LocalFrame directly. A DocumentLoader will be extracted at runtime from this LocalFrame. During the lifetime of a LocalFrame, multiple documents can get loaded and get attached to the same frame, which causes old PNT instance to reference DocumentLoader that could be created for new cross-origin document. Therefore, instead of holding on to a LocalFrame, PNT should hold on to a Document instead. The change in this patch also fixes the crash reported by clusterfuzz, which is dereferencing a null pointer when PerformanceNavigationTiming::type gets called after a Document gets replaced which causes its associated DocumentLoader to be null. BUG=704352, 703540 ==========
The CQ bit was checked by sunjian@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by sunjian@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org, panicker@chromium.org Link to the patchset: https://codereview.chromium.org/2774543003/#ps110001 (title: "sync")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 110001, "attempt_start_ts": 1490725477892050,
"parent_rev": "1b5a3b537f25b71db4b47b80158d1ee4f1a90c32", "commit_rev":
"47b93d128610246960c032a00b67ec2083b2a05b"}
Message was sent while issue was closed.
Description was changed from ========== Fix PerformanceNavigationTiming accessor behavior after document detach. Currently, PerformanceNavigationTiming holds on to a LocalFrame directly. A DocumentLoader will be extracted at runtime from this LocalFrame. During the lifetime of a LocalFrame, multiple documents can get loaded and get attached to the same frame, which causes old PNT instance to reference DocumentLoader that could be created for new cross-origin document. Therefore, instead of holding on to a LocalFrame, PNT should hold on to a Document instead. The change in this patch also fixes the crash reported by clusterfuzz, which is dereferencing a null pointer when PerformanceNavigationTiming::type gets called after a Document gets replaced which causes its associated DocumentLoader to be null. BUG=704352, 703540 ========== to ========== Fix PerformanceNavigationTiming accessor behavior after document detach. Currently, PerformanceNavigationTiming holds on to a LocalFrame directly. A DocumentLoader will be extracted at runtime from this LocalFrame. During the lifetime of a LocalFrame, multiple documents can get loaded and get attached to the same frame, which causes old PNT instance to reference DocumentLoader that could be created for new cross-origin document. Therefore, instead of holding on to a LocalFrame, PNT should hold on to a Document instead. The change in this patch also fixes the crash reported by clusterfuzz, which is dereferencing a null pointer when PerformanceNavigationTiming::type gets called after a Document gets replaced which causes its associated DocumentLoader to be null. BUG=704352, 703540 Review-Url: https://codereview.chromium.org/2774543003 Cr-Commit-Position: refs/heads/master@{#460198} Committed: https://chromium.googlesource.com/chromium/src/+/47b93d128610246960c032a00b67... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:110001) as https://chromium.googlesource.com/chromium/src/+/47b93d128610246960c032a00b67...
Message was sent while issue was closed.
On 2017/03/28 at 20:14:15, commit-bot wrote: > Committed patchset #4 (id:110001) as https://chromium.googlesource.com/chromium/src/+/47b93d128610246960c032a00b67... A GitHub PR was created for this CL: https://github.com/w3c/web-platform-tests/pull/5254 However, due to WPT Travis CI problems it hasn't been exported yet. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
