PlzNavigate: preserve SourceLocation when navigating
This CL ensures we keep track of the SourceLocation information when
navigating, so that it can be used in console error messages generated
at commit time. The information will also be used in a later patch to
generate console error messages for CSP failures computed in the browser
process.
BUG=695072
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2720763002
Cr-Commit-Position: refs/heads/master@{#456739}
Committed: https://chromium.googlesource.com/chromium/src/+/19f01147eb8a5af46027bdbb1c843085fbac339c
Description was changed from ========== PlzNavigate: preserve SourceLocation when navigating This CL ensures we keep ...
3 years, 9 months ago
(2017-02-27 14:39:33 UTC)
#1
Description was changed from
==========
PlzNavigate: preserve SourceLocation when navigating
This CL ensures we keep track of the SourceLocation information when
navigating, so that it can be used in console error messages generated
at commit time. The information will also be used in a later patch to
generate console error messages for CSP failures computed in the browser
process.
BUG=695072
==========
to
==========
PlzNavigate: preserve SourceLocation when navigating
This CL ensures we keep track of the SourceLocation information when
navigating, so that it can be used in console error messages generated
at commit time. The information will also be used in a later patch to
generate console error messages for CSP failures computed in the browser
process.
BUG=695072
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========
clamy
The CQ bit was checked by clamy@chromium.org to run a CQ dry run
3 years, 9 months ago
(2017-02-27 14:39:40 UTC)
#2
Dry run: Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xcode-clang/builds/49841)
3 years, 9 months ago
(2017-02-27 14:42:06 UTC)
#5
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_compile_dbg_ng/builds/246939)
3 years, 9 months ago
(2017-02-27 15:43:16 UTC)
#9
3 years, 9 months ago
(2017-03-01 15:11:16 UTC)
#13
Dry run: This issue passed the CQ dry run.
clamy
Description was changed from ========== PlzNavigate: preserve SourceLocation when navigating This CL ensures we keep ...
3 years, 9 months ago
(2017-03-01 15:55:40 UTC)
#14
Description was changed from
==========
PlzNavigate: preserve SourceLocation when navigating
This CL ensures we keep track of the SourceLocation information when
navigating, so that it can be used in console error messages generated
at commit time. The information will also be used in a later patch to
generate console error messages for CSP failures computed in the browser
process.
BUG=695072
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========
to
==========
PlzNavigate: preserve SourceLocation when navigating
This CL ensures we keep track of the SourceLocation information when
navigating, so that it can be used in console error messages generated
at commit time. The information will also be used in a later patch to
generate console error messages for CSP failures computed in the browser
process.
BUG=695072
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========
3 years, 9 months ago
(2017-03-01 15:55:57 UTC)
#16
@japhet:PTAL
Nate Chapin
https://codereview.chromium.org/2720763002/diff/40001/content/renderer/render_frame_impl.cc File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2720763002/diff/40001/content/renderer/render_frame_impl.cc#newcode6107 content/renderer/render_frame_impl.cc:6107: frame_->dataSource()->resetSourceLocation(); Rather than needing to expose resetSourceLocation() and calling ...
3 years, 9 months ago
(2017-03-01 23:22:19 UTC)
#17
https://codereview.chromium.org/2720763002/diff/40001/content/renderer/render_frame_impl.cc File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2720763002/diff/40001/content/renderer/render_frame_impl.cc#newcode6107 content/renderer/render_frame_impl.cc:6107: frame_->dataSource()->resetSourceLocation(); Rather than needing to expose resetSourceLocation() and calling ...
3 years, 9 months ago
(2017-03-01 23:22:19 UTC)
#18
@japhet: thanks! @nasko: PTAL at the code in content/. https://codereview.chromium.org/2720763002/diff/40001/content/renderer/render_frame_impl.cc File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2720763002/diff/40001/content/renderer/render_frame_impl.cc#newcode6107 content/renderer/render_frame_impl.cc:6107: ...
3 years, 9 months ago
(2017-03-02 13:34:32 UTC)
#20
@japhet: thanks!
@nasko: PTAL at the code in content/.
https://codereview.chromium.org/2720763002/diff/40001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):
https://codereview.chromium.org/2720763002/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:6107:
frame_->dataSource()->resetSourceLocation();
On 2017/03/01 23:22:19, Nate Chapin wrote:
> Rather than needing to expose resetSourceLocation() and calling it here, could
> we just clear it in DocumentLoader on commit?
The problem is that we're going to do an IPC before committing the navigation.
If any other Javascript runs in the meantime and ends up emitting a console
message, we will have the wrong SourceLocation set.
https://codereview.chromium.org/2720763002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):
https://codereview.chromium.org/2720763002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:5815:
m_frame->loader().provisionalDocumentLoader();
On 2017/03/01 23:22:19, Nate Chapin wrote:
> This seems wrong. Anything going through a Document should be associated with
> the committed DocumentLoader, not the provisional DocumentLoader. What depends
> on this?
I thought the message I whose SourceLocation I needed to change went through
this function, but actually it goes directly through the FrameConsole. Removed
that part.
https://codereview.chromium.org/2720763002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/LocalFrameClientImpl.cpp (right):
https://codereview.chromium.org/2720763002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/LocalFrameClientImpl.cpp:577:
navigationInfo.hasSourceLocation = true;
On 2017/03/01 23:22:19, Nate Chapin wrote:
> Rather than have hasSourceLocation, could we either have
> WebSourceLocation::isUnknown(), or just check WebSourceLocation.url.isNull()?
Done.
nasko
content/ LGTM with a few comments. https://codereview.chromium.org/2720763002/diff/80001/content/browser/frame_host/navigation_request.cc File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2720763002/diff/80001/content/browser/frame_host/navigation_request.cc#newcode531 content/browser/frame_host/navigation_request.cc:531: // Allow the ...
3 years, 9 months ago
(2017-03-02 23:56:05 UTC)
#21
https://codereview.chromium.org/2720763002/diff/40001/content/renderer/render_frame_impl.cc File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2720763002/diff/40001/content/renderer/render_frame_impl.cc#newcode6107 content/renderer/render_frame_impl.cc:6107: frame_->dataSource()->resetSourceLocation(); On 2017/03/02 13:34:32, clamy wrote: > On 2017/03/01 ...
3 years, 9 months ago
(2017-03-03 00:17:05 UTC)
#22
https://codereview.chromium.org/2720763002/diff/40001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):
https://codereview.chromium.org/2720763002/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:6107:
frame_->dataSource()->resetSourceLocation();
On 2017/03/02 13:34:32, clamy wrote:
> On 2017/03/01 23:22:19, Nate Chapin wrote:
> > Rather than needing to expose resetSourceLocation() and calling it here,
could
> > we just clear it in DocumentLoader on commit?
>
> The problem is that we're going to do an IPC before committing the navigation.
> If any other Javascript runs in the meantime and ends up emitting a console
> message, we will have the wrong SourceLocation set.
So the flow of that case would be:
* Start navigation that sets a SourceLocation on the provisionalDataSource().
* setTimeout() timer fires, JS runs and sends console message
* Data arrives from browser process, data source commits.
Is that right? If so, wouldn't the JS be associated with the committed
WebDataSource/DocumentLoader, instead of the provisional, so it would safely
ignore the SourceLocation? Or did I misunderstand?
clamy
The CQ bit was checked by clamy@chromium.org to run a CQ dry run
3 years, 9 months ago
(2017-03-03 13:33:07 UTC)
#23
3 years, 9 months ago
(2017-03-06 13:27:53 UTC)
#29
Thanks!
https://codereview.chromium.org/2720763002/diff/40001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):
https://codereview.chromium.org/2720763002/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:6107:
frame_->dataSource()->resetSourceLocation();
On 2017/03/03 00:17:05, Nate Chapin wrote:
> On 2017/03/02 13:34:32, clamy wrote:
> > On 2017/03/01 23:22:19, Nate Chapin wrote:
> > > Rather than needing to expose resetSourceLocation() and calling it here,
> could
> > > we just clear it in DocumentLoader on commit?
> >
> > The problem is that we're going to do an IPC before committing the
navigation.
> > If any other Javascript runs in the meantime and ends up emitting a console
> > message, we will have the wrong SourceLocation set.
>
> So the flow of that case would be:
> * Start navigation that sets a SourceLocation on the provisionalDataSource().
> * setTimeout() timer fires, JS runs and sends console message
> * Data arrives from browser process, data source commits.
>
> Is that right? If so, wouldn't the JS be associated with the committed
> WebDataSource/DocumentLoader, instead of the provisional, so it would safely
> ignore the SourceLocation? Or did I misunderstand?
That's the case I'm concerned about. Given how the code is written, I don't see
why we would associate it with the committed DataSource, since we override the
location when the provisional DataSource has a SourceLocation.
I do think that the correct thing to do is to only have the SourceLocation info
in the provisionalDataSource for one iteration of the message loop, which is
what this code is doing. After all, IIUC in the current code, you would not get
the SourceLocation for the navigation after the Javascript that triggered it
stopped executing, that is we switched to a new iteration in the message loop?
https://codereview.chromium.org/2720763002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.cc (right):
https://codereview.chromium.org/2720763002/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:531: // Allow the embedder to
cancel the transfer if needed.
On 2017/03/02 23:56:04, nasko (slow) wrote:
> nit: Technically there are no transfers in PlzNavigate, right? And
> NavigationRequest is PlzNavigate specific object. The method is named with
> transfer, but I think this can be renamed/improved later on once we ship.
No indeed. I've rephrased the comment to remove the word transfer and added a
todo to rename the function once PlzNavigate ships.
https://codereview.chromium.org/2720763002/diff/80001/content/common/navigati...
File content/common/navigation_params.h (right):
https://codereview.chromium.org/2720763002/diff/80001/content/common/navigati...
content/common/navigation_params.h:141: // Information about the javascript
source for this navigation. Used for
On 2017/03/02 23:56:04, nasko (slow) wrote:
> nit: Be consistent with the capitalization of "Javascript" (that being the
> casing in the first comment and next sentence).
Done.
https://codereview.chromium.org/2720763002/diff/80001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):
https://codereview.chromium.org/2720763002/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:6109:
frame_->provisionalDataSource()->resetSourceLocation();
On 2017/03/02 23:56:04, nasko (slow) wrote:
> Should we clear it here or in didCommitProvisionalLoad?
This is what we're discussing with Nate in patchset 3. I think we should only
keep it for one iteration of the message loop, hence why I clear it here. This
way, we won;t use this info if a Javascript that executes before commit creates
a console message.
https://codereview.chromium.org/2720763002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation
(left):
https://codereview.chromium.org/2720763002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation:23:
crbug.com/555418
virtual/mojo-loading/http/tests/security/contentSecurityPolicy/redirect-does-not-match-paths.html
[ Timeout ]
On 2017/03/02 23:56:05, nasko (slow) wrote:
> Are these really passing with just this fix? If yes, the are likely
incorrectly
> classified with the comment above.
Yes they were incorrectly classified.
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
3 years, 9 months ago
(2017-03-06 14:53:20 UTC)
#30
3 years, 9 months ago
(2017-03-06 14:53:21 UTC)
#31
Dry run: This issue passed the CQ dry run.
clamy
@japhet: friendly ping :)
3 years, 9 months ago
(2017-03-09 12:20:07 UTC)
#32
@japhet: friendly ping :)
clamy
@japhet: ping?
3 years, 9 months ago
(2017-03-13 13:48:02 UTC)
#33
@japhet: ping?
Nate Chapin
Sorry for the delay. LGTM. https://codereview.chromium.org/2720763002/diff/40001/content/renderer/render_frame_impl.cc File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2720763002/diff/40001/content/renderer/render_frame_impl.cc#newcode6107 content/renderer/render_frame_impl.cc:6107: frame_->dataSource()->resetSourceLocation(); On 2017/03/06 13:27:53, ...
3 years, 9 months ago
(2017-03-13 17:45:42 UTC)
#34
Sorry for the delay. LGTM.
https://codereview.chromium.org/2720763002/diff/40001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):
https://codereview.chromium.org/2720763002/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:6107:
frame_->dataSource()->resetSourceLocation();
On 2017/03/06 13:27:53, clamy wrote:
> On 2017/03/03 00:17:05, Nate Chapin wrote:
> > On 2017/03/02 13:34:32, clamy wrote:
> > > On 2017/03/01 23:22:19, Nate Chapin wrote:
> > > > Rather than needing to expose resetSourceLocation() and calling it here,
> > could
> > > > we just clear it in DocumentLoader on commit?
> > >
> > > The problem is that we're going to do an IPC before committing the
> navigation.
> > > If any other Javascript runs in the meantime and ends up emitting a
console
> > > message, we will have the wrong SourceLocation set.
> >
> > So the flow of that case would be:
> > * Start navigation that sets a SourceLocation on the
provisionalDataSource().
> > * setTimeout() timer fires, JS runs and sends console message
> > * Data arrives from browser process, data source commits.
> >
> > Is that right? If so, wouldn't the JS be associated with the committed
> > WebDataSource/DocumentLoader, instead of the provisional, so it would safely
> > ignore the SourceLocation? Or did I misunderstand?
>
> That's the case I'm concerned about. Given how the code is written, I don't
see
> why we would associate it with the committed DataSource, since we override the
> location when the provisional DataSource has a SourceLocation.
>
> I do think that the correct thing to do is to only have the SourceLocation
info
> in the provisionalDataSource for one iteration of the message loop, which is
> what this code is doing. After all, IIUC in the current code, you would not
get
> the SourceLocation for the navigation after the Javascript that triggered it
> stopped executing, that is we switched to a new iteration in the message loop?
Ok, I think I can live with this. The combination of the special case in
FrameConsole.cpp and restricting it to a single message loop here is a bit
difficult to reason about. But I think the alternative is to audit all callers
of FrameConsole::addMessage() and figure out which ones should be sensitive to
the provisionalDocumentLoader()'s SourceLocation, and there may be some gray
areas.
clamy
The CQ bit was checked by clamy@chromium.org
3 years, 9 months ago
(2017-03-14 10:28:18 UTC)
#35
Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xcode-clang/builds/58793)
3 years, 9 months ago
(2017-03-14 10:31:00 UTC)
#40
Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_android/builds/227326)
3 years, 9 months ago
(2017-03-14 14:52:23 UTC)
#45
CQ is committing da patch. Bot data: {"patchset_id": 180001, "attempt_start_ts": 1489506079935660, "parent_rev": "49a841a4fa2889833c8181cc18eae546db9370f7", "commit_rev": "19f01147eb8a5af46027bdbb1c843085fbac339c"}
3 years, 9 months ago
(2017-03-14 17:05:05 UTC)
#49
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1489506079935660,
"parent_rev": "49a841a4fa2889833c8181cc18eae546db9370f7", "commit_rev":
"19f01147eb8a5af46027bdbb1c843085fbac339c"}
commit-bot: I haz the power
Description was changed from ========== PlzNavigate: preserve SourceLocation when navigating This CL ensures we keep ...
3 years, 9 months ago
(2017-03-14 17:05:46 UTC)
#50
Message was sent while issue was closed.
Description was changed from
==========
PlzNavigate: preserve SourceLocation when navigating
This CL ensures we keep track of the SourceLocation information when
navigating, so that it can be used in console error messages generated
at commit time. The information will also be used in a later patch to
generate console error messages for CSP failures computed in the browser
process.
BUG=695072
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========
to
==========
PlzNavigate: preserve SourceLocation when navigating
This CL ensures we keep track of the SourceLocation information when
navigating, so that it can be used in console error messages generated
at commit time. The information will also be used in a later patch to
generate console error messages for CSP failures computed in the browser
process.
BUG=695072
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2720763002
Cr-Commit-Position: refs/heads/master@{#456739}
Committed:
https://chromium.googlesource.com/chromium/src/+/19f01147eb8a5af46027bdbb1c84...
==========
commit-bot: I haz the power
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/19f01147eb8a5af46027bdbb1c843085fbac339c
3 years, 9 months ago
(2017-03-14 17:05:48 UTC)
#51
Issue 2720763002: PlzNavigate: preserve SourceLocation when navigating
(Closed)
Created 3 years, 9 months ago by clamy
Modified 3 years, 9 months ago
Reviewers: Nate Chapin, nasko
Base URL:
Comments: 17
Chromium Code Reviews
Issue
2720763002:
PlzNavigate: preserve SourceLocation when navigating (Closed)
