Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2531013002:
WebView loadDataWithBaseURL() was determined as a same page navigation (Closed)
|
Created:
4 years ago by Takashi Toyoshima Modified:
4 years ago CC:
chromium-reviews, tyoshino+watch_chromium.org, loading-reviews_chromium.org, gavinp+loader_chromium.org, blink-reviews, kinuko+watch, Nate Chapin, android-webview-reviews_chromium.org, boliu Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWebView loadDataWithBaseURL() was determined as a same page navigation
In WebView, if loadDataWithBaseURL() is called with null or non-data
baseUrl consequently, previous historyUrl and new baseUrl is compared
to detemine a same page navigation. To be consistent with others, we
need to use the new URL for the comparison, but it isn't clear what
should be URL for loadDataWithBaseURL().
In this patch, we assume loadDataWithBaseURL() does not make a same page
navigation always to be compatible in most API usages.
BUG=662823
Patch Set 1 #
Total comments: 5
Messages
Total messages: 34 (7 generated)
The CQ bit was checked by toyoshim@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.
Description was changed from ========== WebView loadDataWithBaseURL() was determined as a same page navigation In WebView, if loadDataWithBaseURL() is called with null or non-data baseUrl consequently, previous historyUrl and new baseUrl is compared to detemine a same page navigation. To be consistent with others, we need to use the new URL for the comparison, but it isn't clear what should be URL for loadDataWithBaseURL(). In this patch, we assume loadDataWithBaseURL() does not make a same page navigation always to be compatible in most API usages. BUG=662823 ========== to ========== WebView loadDataWithBaseURL() was determined as a same page navigation In WebView, if loadDataWithBaseURL() is called with null or non-data baseUrl consequently, previous historyUrl and new baseUrl is compared to detemine a same page navigation. To be consistent with others, we need to use the new URL for the comparison, but it isn't clear what should be URL for loadDataWithBaseURL(). In this patch, we assume loadDataWithBaseURL() does not make a same page navigation always to be compatible in most API usages. BUG=662823 ==========
toyoshim@chromium.org changed reviewers: + creis@chromium.org, torne@chromium.org
Hi, can you take a look? creis@ do you think this is a right fix in terms of navigation? trone@ is this approach fine with WebView?
This LGTM from a WebView perspective; this just means webview will be back to the old state (no scroll/etc restoration) when using loadDataWithBaseURL, correct?
This LGTM from a WebView perspective; this just means webview will be back to the old state (no scroll/etc restoration) when using loadDataWithBaseURL, correct?
creis@chromium.org changed reviewers: + dcheng@chromium.org
[+dcheng] Sorry, I'm out of my depth here (and not an owner in Blink). I would defer to Nate but he's out on leave. Daniel, are you able to review this? In general, I tend to not care too much about loadDataWithBaseURL fixes if they pass the tests, but we should understand whether this affects other cases or not. (It does feel like this is a feature that desperately needs refactoring, but I certainly won't ask for that in this CL.) https://codereview.chromium.org/2531013002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2531013002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/FrameLoader.cpp:942: // TODO(crbug.com/662823): WebFrameLoaderImpl::loadData() sets baseUrl for Can you clarify what still needs to be done here? I would also suggest filing a separate bug for it, since I expect that we'll be closing 662823 when this CL lands. https://codereview.chromium.org/2531013002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/FrameLoader.cpp:945: bool isDataUrlWithEmulatedBaseUrl = request.substituteData().isValid(); nit: isDataUrlWithBaseUrl seems like it would be more consistent with the API name. I'm mildly concerned that checking for this via substituteData might affect more cases than just loadDataWithBaseURL. I'm not familiar with what SubstituteData is used for, so I can't say.
https://codereview.chromium.org/2531013002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2531013002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/FrameLoader.cpp:945: bool isDataUrlWithEmulatedBaseUrl = request.substituteData().isValid(); I echo Charlie's concern here: is this actually selective to only the data URL with base URL case? I see SubstituteData() created in a number of other places (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/fetch/Sub...), so it's not clear to me that this filtering is selective enough (or maybe it is, but needs comments to explain why)
I filed a new bug crbug.com/669408 as creis@ suggested. Torne, can you join the thread? The WebView document looks a little unclear to me in following points. - What the page URL should be? Same URL with baseUrl, or encoded content in a data URL format? - The last note the baseUrl is sent as a referer seems to imply baseUrl is used for URL. Without understanding this, I could not say what is the right fix here actually.
https://codereview.chromium.org/2531013002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2531013002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/FrameLoader.cpp:942: // TODO(crbug.com/662823): WebFrameLoaderImpl::loadData() sets baseUrl for Filed a new bug, crbug.com/669408. Actually, this comment wasn't enough to explain everything. Sorry for that. As I told torne@, WebView spec looks a little unclear to me. The document does not explain what should be the page URL for this API. Since we need the page URL to detect the same page navigation correctly, I just tried to exclude all loadDataWithBaseURL() cases from the same page navigation in this patch as a workaround. If the URL should be the same URL with baseUrl, current behavior should be a right expected one. If the URL should be encoded content in a data URL format, we may need exceptional code to send the baseURL as a referer for this WebView API. IMO, assuming baseUrl as a page URL sounds more reasonable. https://codereview.chromium.org/2531013002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/FrameLoader.cpp:945: bool isDataUrlWithEmulatedBaseUrl = request.substituteData().isValid(); Agreed. Probably, we should discuss what is the right behavior first. Then, if we decided to handle loadDataWithBaseURL() cases in a special way like this, I'd introduce an explicit flag in the SubstituteData() to say this content is generated by loadDataWithBaseURL().
On 2016/11/29 09:13:33, toyoshim wrote: > I filed a new bug crbug.com/669408 as creis@ suggested. > Torne, can you join the thread? > > The WebView document looks a little unclear to me in following points. > > - What the page URL should be? Same URL with baseUrl, or encoded content in a > data URL format? > - The last note the baseUrl is sent as a referer seems to imply baseUrl is used > for URL. > > Without understanding this, I could not say what is the right fix here actually. I don't know what you mean by "page URL" in this context, and the only reference to know what the expected behaviour is supposed to be is probably "whatever webview currently does" - webview's API needs to be compatible with the behaviour of older versions in general because we have no real way to know what details applications are relying on. I'm not sure what else I can answer without more specific context for what you mean, sorry.
IMO, Blink should not make much efforts to keep minor old behaviors that are not defined in the spec or documented well, especially when it just relied on (wrong) implementation. Also if users are less than 0.03%, we can consider to drop it. The page URL I mentioned here is the |URL| that the navigation spec uses; https://www.w3.org/TR/html5/browsers.html#browsing-the-web In current implementation loadDataWithBaseURL() sets the baseUrl to the page URL even though baseUrl and URL are different concept. Unfortunately, the Android document says this baseUrl is used as HTTP referer. This is incompatible with the web, and I guess it just excuses its wrong implementation, setting baseUrl to the page URL results in using it as HTTP referer. For instance, in the page loaded by loadDataWithBaseURL(), location.href will return the baseUrl, right? But if you want to set only baseUrl, it should affect only document.baseURI. It should not change location.href, and HTTP referer. I'm not sure if this is the original behavior or a bug introduced when it switched to use Blink.
On 2016/11/30 06:24:56, toyoshim wrote: > IMO, Blink should not make much efforts to keep minor old behaviors that are not > defined in the spec or documented well, especially when it just relied on > (wrong) implementation. By "minor old behaviour" do you mean the scroll restoration behaviour specifically? That's something we had a developer file a bug about, which is a good sign it may be a compatibility concern for WebView. We don't have an easy way to determine whether this is actually minor or not for the ecosystem at large :/ > Also if users are less than 0.03%, we can consider to > drop it. WebView has no way to gather metrics, and it's not clear how we would measure usage of this anyway - how would we know when we'd hit the problematic case? > The page URL I mentioned here is the |URL| that the navigation spec uses; > https://www.w3.org/TR/html5/browsers.html#browsing-the-web > > In current implementation loadDataWithBaseURL() sets the baseUrl to the page URL > even though baseUrl and URL are different concept. Unfortunately, the Android > document says this baseUrl is used as HTTP referer. This is incompatible with > the web, and I guess it just excuses its wrong implementation, setting baseUrl > to the page URL results in using it as HTTP referer. For instance, in the page > loaded by loadDataWithBaseURL(), location.href will return the baseUrl, right? > But if you want to set only baseUrl, it should affect only document.baseURI. It > should not change location.href, and HTTP referer. I'm not sure if this is the > original behavior or a bug introduced when it switched to use Blink. You're overlooking a much more important effect of the base URL, which is that it's used as the actual page origin for security purposes. Just setting document.baseURI wouldn't work at all. The purpose of loadDataWithBaseURL is to be able to load a chunk of data the app already has as if it were a page loaded from a real HTTP location. This means it has to have the correct origin (for example, to allow XMLHttpRequest to the real webserver), relative URLs have to be loaded relative to that URL, the referrer for embedded resources must be that URL (in case the site has referrer-based access control), and for consistency it does make sense that location.href is also that URL. A better name for this API might have been "loadFakeDataForRealURL(String realURL, String data, ...)", to avoid confusing things with document.baseURI. This is certainly how applications use it and expect it to work. I'm not sure what you mean here by "wrong implementation", and I'm fairly sure this was always the behaviour of WebView - however even if it wasn't, the Chromium WebView has now been out there in the wild for sufficiently long that there apps that *also* depend on any behaviours we accidentally changed in the transition, and so even inconsistency with the pre-chromium WebView isn't necessarily an automatic pass to change behaviour.
> You're overlooking a much more important effect of the base URL, which is that > it's used as the actual page origin for security purposes. Just setting > document.baseURI wouldn't work at all. So it isn't baseUrl any more. What the API actually provides is to set the page URL. That's what I asked. API name and document say it's baseUrl, but it's the URL in the real implementation. If we want to respect actual implementation and behaviors, we can assume baseUrl as a page URL.
On 2016/11/30 10:36:03, toyoshim wrote: > > You're overlooking a much more important effect of the base URL, which is that > > it's used as the actual page origin for security purposes. Just setting > > document.baseURI wouldn't work at all. > > So it isn't baseUrl any more. What the API actually provides is to set the page > URL. That's what I asked. API name and document say it's baseUrl, but it's the > URL in the real implementation. If we want to respect actual implementation and > behaviors, we can assume baseUrl as a page URL. Do I understand correctly that the page URL being the same is what normally determines a same page navigation though? So wouldn't that mean that the case the bug was about would still restore the scroll position? Maybe it's not really loadDataWithBaseURL that's the actual issue with scroll restoration, thinking about it :/ The problem in the bug is that the developer doesn't care about the URL at all (and so just passes null), and expects that because the data has changed it will be treated as a fresh page load. If there was a real URL being used, though, then I'm not sure it would be surprising for two calls with the same URL to be treated as a same-page navigation. Maybe the actual issue is that the use case of "about:blank used as a starting point to inject arbitrary content from external code" (which you can do with iframes on regular web content as well, no? this isn't specific to webview) is a special case to begin with?
I checked an old API document from SDK 1.0r2, and am a bit confused actually. > void loadDataWithBaseURL (String baseUrl, String data, String mimeType, String encoding, String failUrl); > > (snip) The base URL is the URL that represents the page that is loaded through this interface. As such, it is used for the history entry and to resolve any relative URLs. But current document says > void loadDataWithBaseURL (String baseUrl, String data, String mimeType, String encoding, String historyUrl); So it seems definition of the last String argument was changed at some points. I can read the original document as the baseUrl is virtually assumed as a page URL, and it should be used for the history, too. But recent API document does not say it, and last argument was just changed to set the history URL instead of using baseUrl. This explains why Blink set the historyUrl to failUrl internally. What's the expected implementation/behavior here? Does this work correctly as documents say?
On 2016/11/30 10:51:53, toyoshim wrote: > I checked an old API document from SDK 1.0r2, and am a bit confused actually. > > > void loadDataWithBaseURL (String baseUrl, String data, String mimeType, String > encoding, String failUrl); > > > > (snip) The base URL is the URL that represents the page that is loaded through > this interface. As such, it is used for the history entry and to resolve any > relative URLs. Such an old SDK doesn't really matter to anything now. Any behaviour that dates back to before around SDK 9 (gingerbread) isn't likely to be relevant, as developers haven't been targeting such old versions for a long time, and so the behaviour on more recent versions is the "canonical" behaviour. > But current document says > > > void loadDataWithBaseURL (String baseUrl, String data, String mimeType, String > encoding, String historyUrl); > > So it seems definition of the last String argument was changed at some points. I > can read the original document as the baseUrl is virtually assumed as a page > URL, and it should be used for the history, too. But recent API document does > not say it, and last argument was just changed to set the history URL instead of > using baseUrl. > > This explains why Blink set the historyUrl to failUrl internally. What's the > expected implementation/behavior here? Does this work correctly as documents > say? The expected behaviour is whatever WebView does, as I said - that's how app compatibility has to work. I don't think WebView's behaviour here has actually changed for a long time, and I'm fairly sure the pre-chromium webview behaved the same way here. Whether it matches the documentation is much less important and we would typically fix the documentation in cases where the behaviour has been different for a long enough time that apps are relying on it.
The current function signature and documentation have been there since at least Froyo (the oldest version I can easily check), so yeah, any previous version is totally irrelevant and it doesn't seem likely that the behaviour has actually changed in any significant way since Froyo.
Re#19 Regardless of my CL, reported case has been assumed as a same page navigation in Blink. That's why my CL affects this case as a result of expected behavior change. It's same with the case a page is reloaded, and scroll position is restored though content is modified. > If there was a real URL being used, though, then I'm not sure it would be surprising for two calls with the same URL to be treated as a same-page navigation. Actually we have detected such cases as a same-page navigation from the WebKit era, though same page navigation's behaviors were different from today. But to solve such cache problems, we have a cache control protocol in HTTP. It does not have a power for data URL, but in case of data URL, the URL can be changed if content is changed. That's equivalent to assign a unique ID for each unique content, and work fine to avoid being cached. If I am a user, I'd call loadData() with an arbitrary content with <base> tag to specify the base URL. But it does not overwrite CORS checks. So, if a user uses loadDataWithBaseURL() to bypass the CORS check, it won't work. For iframe case, we have some alternatives, e.g. encoding arbitrary content into data URL, calling URL.createObjectURL for generated Blob/ArrayBuffer/etc. But we can not bypass CORS check of course for security.
Re #21 > The expected behaviour is whatever WebView does, as I said - that's how app > compatibility has to work So do you have a chance to update the API document correctly to match with actual implementation? Also, can you have metrics in the future to discuss this kind of problems productively? Blink developers could not pay much attentions to keep undocumented behaviors for a long period only for WebView. We always need large code changes to improve the web, adding features, optimizing speed, and so on. It may change old behaviors unexpectedly. Or we may even intentionally make changes to fix wrong/bad/inconsistent/etc things or to be aligned with other browsers. So ideally, we need a spec and layout/unit tests in Blink if something should not be changed. I feel the goal and priority are different between Blink and WebView. That's acceptable, and we both have enough reasons. But IMO, moving forward with minimum compatibility breaks is better than sticking old behaviors. That had been web developers' dream until some years ago.
On 2016/11/30 11:21:51, toyoshim wrote:
> Re#19
>
> Regardless of my CL, reported case has been assumed as a same page navigation
in
> Blink.
> That's why my CL affects this case as a result of expected behavior change.
It's
> same with the case a page is reloaded, and scroll position is restored though
> content is modified.
Yes, I understand that - the issue is that before your CL it didn't matter to
the developer whether it was treated as a same page navigation in Blink or not,
because there wasn't any observable behaviour difference, but now with your CL
it makes a difference.
> > If there was a real URL being used, though, then I'm not sure it would be
> surprising for two calls with the same URL to be treated as a same-page
> navigation.
>
> Actually we have detected such cases as a same-page navigation from the WebKit
> era, though same page navigation's behaviors were different from today.
> But to solve such cache problems, we have a cache control protocol in HTTP. It
> does not have a power for data URL, but in case of data URL, the URL can be
> changed if content is changed. That's equivalent to assign a unique ID for
each
> unique content, and work fine to avoid being cached.
I don't think you understand what I meant here, as I'm not sure how your
response relates. What I meant is that in the case of a sequence like:
loadDataWithBaseUrl("http://foo.com", data, ...);
loadDataWithBaseUrl("http://foo.com", different_data, ...);
it probably *is* fine that the scroll/zoom are restored, and probably even
actively desirable in some cases, because this *is* semantically like reloading
the page from the server and just getting different content.
The case that seems like a problem is *just* the case where the URL is null
(about:blank) because that doesn't really seem like it's actually a same-page
navigation in any meaningful way - there's no reason to really assume that the
page is related in any way if it's just arbitrary content injected into
about:blank.
This is just me musing about what the behaviour should actually be here.. I'm
just wondering what you think.
> If I am a user, I'd call loadData() with an arbitrary content with <base> tag
to
> specify the base URL. But it does not overwrite CORS checks. So, if a user
uses
> loadDataWithBaseURL() to bypass the CORS check, it won't work.
> For iframe case, we have some alternatives, e.g. encoding arbitrary content
into
> data URL, calling URL.createObjectURL for generated Blob/ArrayBuffer/etc. But
we
> can not bypass CORS check of course for security.
Sure, but the embedder in WebView is already all-powerful (they can also
substitute the contents of any network request), so letting them do things like
"load content generated by the app in such a way that it can still make XHRs to
the app's corresponding API server" (which is indeed "bypass CORS" when you look
at it) seems reasonable.
On 2016/11/30 12:12:57, toyoshim wrote: > Re #21 > > The expected behaviour is whatever WebView does, as I said - that's how app > > compatibility has to work > > So do you have a chance to update the API document correctly to match with > actual implementation? Yes, when we find problems we update the API documentation. This sounds like it would indeed be a good idea to go back and verify what various expected behaviour of loadDataWithBaseURL actually is by looking at how it works and has worked in past versions, and clarify the documentation to describe the behaviour more accurately. I've filed crbug.com/669885 to track this. > Also, can you have metrics in the future to discuss this kind of problems > productively? This hasn't been possible due to the way WebView is embedded in other applications (privacy/security concerns, lack of any guaranteed network access, etc) - we are looking into it at present and but aren't currently sure on what timeline we'll be able to start gathering UMA metrics. Even if we had this capability, however, it's really unclear to me what metric we would look at to make a decision about the issue we're talking about here: how can metrics know whether things that look like same-page navigations were intended to be treated that way or not? > Blink developers could not pay much attentions to keep undocumented behaviors > for a long period only for WebView. > We always need large code changes to improve the web, adding features, > optimizing speed, and so on. It may change old behaviors unexpectedly. > Or we may even intentionally make changes to fix wrong/bad/inconsistent/etc > things or to be aligned with other browsers. > So ideally, we need a spec and layout/unit tests in Blink if something should > not be changed. > > I feel the goal and priority are different between Blink and WebView. That's > acceptable, and we both have enough reasons. > But IMO, moving forward with minimum compatibility breaks is better than > sticking old behaviors. That had been web developers' dream until some years > ago. I understand that Android's conservative approach to API compatibility is a problem, and we frequently *do* ship changes to WebView that are compatibility breaks when the evidence suggests that the impact on the ecosystem is minor, or at least much smaller than the benefits. When I originally reviewed your CL I tried to explain that the current situation was a possible outcome: that the behavioural change caused by your CL might result in bug reports from WebView developers and that if that happened, we may have to reconsider the change. We aren't yet in a position to come to a really firm conclusion about whether this change is too much of a compatibility break or not - M55 hasn't been shipped to stable, and we are unfortunately aware that we get much less feedback in beta than we would like. We're not going to block launching M55 on fixing this, but once M55 goes to stable it's possible we will receive more reports of apps that are negatively affected. I really don't know what the best thing to do here is. It seems to me that scroll position restoration in the case of the baseURL and historyURL being about:blank (i.e. null) is actually surprising and unlikely to be what any developer actually wants. In the case of the URL being a real URL, I think it's a much more open question..
On 2016/11/30 14:19:32, Torne wrote: > On 2016/11/30 12:12:57, toyoshim wrote: > > Re #21 > > > The expected behaviour is whatever WebView does, as I said - that's how app > > > compatibility has to work > > > > So do you have a chance to update the API document correctly to match with > > actual implementation? > > Yes, when we find problems we update the API documentation. This sounds like it > would indeed be a good idea to go back and verify what various expected > behaviour of loadDataWithBaseURL actually is by looking at how it works and has > worked in past versions, and clarify the documentation to describe the behaviour > more accurately. I've filed crbug.com/669885 to track this. > > > Also, can you have metrics in the future to discuss this kind of problems > > productively? > > This hasn't been possible due to the way WebView is embedded in other > applications (privacy/security concerns, lack of any guaranteed network access, > etc) - we are looking into it at present and but aren't currently sure on what > timeline we'll be able to start gathering UMA metrics. Even if we had this > capability, however, it's really unclear to me what metric we would look at to > make a decision about the issue we're talking about here: how can metrics know > whether things that look like same-page navigations were intended to be treated > that way or not? > > > Blink developers could not pay much attentions to keep undocumented behaviors > > for a long period only for WebView. > > We always need large code changes to improve the web, adding features, > > optimizing speed, and so on. It may change old behaviors unexpectedly. > > Or we may even intentionally make changes to fix wrong/bad/inconsistent/etc > > things or to be aligned with other browsers. > > So ideally, we need a spec and layout/unit tests in Blink if something should > > not be changed. > > > > I feel the goal and priority are different between Blink and WebView. That's > > acceptable, and we both have enough reasons. > > But IMO, moving forward with minimum compatibility breaks is better than > > sticking old behaviors. That had been web developers' dream until some years > > ago. > > I understand that Android's conservative approach to API compatibility is a > problem, and we frequently *do* ship changes to WebView that are compatibility > breaks when the evidence suggests that the impact on the ecosystem is minor, or > at least much smaller than the benefits. When I originally reviewed your CL I > tried to explain that the current situation was a possible outcome: that the > behavioural change caused by your CL might result in bug reports from WebView > developers and that if that happened, we may have to reconsider the change. We > aren't yet in a position to come to a really firm conclusion about whether this > change is too much of a compatibility break or not - M55 hasn't been shipped to > stable, and we are unfortunately aware that we get much less feedback in beta > than we would like. > > We're not going to block launching M55 on fixing this, but once M55 goes to > stable it's possible we will receive more reports of apps that are negatively > affected. > > I really don't know what the best thing to do here is. It seems to me that > scroll position restoration in the case of the baseURL and historyURL being > about:blank (i.e. null) is actually surprising and unlikely to be what any > developer actually wants. In the case of the URL being a real URL, I think it's > a much more open question.. So I agree with toyoshim: we should rename/clarify the Blink API to make it clear that we're supporting a WebView hack and what the hack is. It wasn't clear to me that this is used to support spoofing an origin. Horrible suggestion for fix: When null is passed as the base URL / history URL, generate a random URL to use instead =P
On 2016/12/02 19:17:14, dcheng wrote: > So I agree with toyoshim: we should rename/clarify the Blink API to make it > clear that we're supporting a WebView hack and what the hack is. It wasn't clear > to me that this is used to support spoofing an origin. Probably a good idea if WebView is the only client of this Blink API. > Horrible suggestion for fix: > When null is passed as the base URL / history URL, generate a random URL to use > instead =P That would probably be a *more* significant compatibility break, unfortunately :)
On 2016/12/05 14:49:14, Torne wrote: > On 2016/12/02 19:17:14, dcheng wrote: > > So I agree with toyoshim: we should rename/clarify the Blink API to make it > > clear that we're supporting a WebView hack and what the hack is. It wasn't > clear > > to me that this is used to support spoofing an origin. > > Probably a good idea if WebView is the only client of this Blink API. > > > Horrible suggestion for fix: > > When null is passed as the base URL / history URL, generate a random URL to > use > > instead =P > > That would probably be a *more* significant compatibility break, unfortunately > :) Why? If the caller is passing in null, why do they care what the base URL is?
On 2016/12/05 18:24:31, dcheng wrote: > On 2016/12/05 14:49:14, Torne wrote: > > On 2016/12/02 19:17:14, dcheng wrote: > > > So I agree with toyoshim: we should rename/clarify the Blink API to make it > > > clear that we're supporting a WebView hack and what the hack is. It wasn't > > clear > > > to me that this is used to support spoofing an origin. > > > > Probably a good idea if WebView is the only client of this Blink API. > > > > > Horrible suggestion for fix: > > > When null is passed as the base URL / history URL, generate a random URL to > > use > > > instead =P > > > > That would probably be a *more* significant compatibility break, unfortunately > > :) > > Why? If the caller is passing in null, why do they care what the base URL is? The docs say that passing in null defaults to "about:blank", so an app may well rely on this.
On 2016/12/05 18:26:02, Torne wrote: > On 2016/12/05 18:24:31, dcheng wrote: > > On 2016/12/05 14:49:14, Torne wrote: > > > On 2016/12/02 19:17:14, dcheng wrote: > > > > So I agree with toyoshim: we should rename/clarify the Blink API to make > it > > > > clear that we're supporting a WebView hack and what the hack is. It wasn't > > > clear > > > > to me that this is used to support spoofing an origin. > > > > > > Probably a good idea if WebView is the only client of this Blink API. > > > > > > > Horrible suggestion for fix: > > > > When null is passed as the base URL / history URL, generate a random URL > to > > > use > > > > instead =P > > > > > > That would probably be a *more* significant compatibility break, > unfortunately > > > :) > > > > Why? If the caller is passing in null, why do they care what the base URL is? > > The docs say that passing in null defaults to "about:blank", so an app may well > rely on this. A top-level about:blank page effectively has unique origin... but I suppose it will be observable via document.URL. I'm finding that the restrictions imposed by the WebView API are quite burdensome to devising a satisfactory solution to this bug...
On 2016/12/05 18:27:49, dcheng wrote: > A top-level about:blank page effectively has unique origin... but I suppose it > will be observable via document.URL. The fact that a top-level about:blank page effectively has unique origin is why I proposed it doesn't seem entirely unreasonable to *not* treat that case as a same-page navigation and not restore the scroll/zoom, because either the about:blank pages are actually blank in which case the scroll/zoom aren't really significant anyway, or they've had content injected into them via some mechanism at which point that content is.. probably different content. Is that too special a special case, or does that seem reasonable? I'm not sure whether I really like this idea or not. > I'm finding that the restrictions imposed by the WebView API are quite > burdensome to devising a satisfactory solution to this bug... Yes, unfortunately the WebView API here evolved around a pretty ad hoc set of app requirements and has weird defaults :( Since we've only had one bug about this even though it's been released to stable for some weeks already, it may be okay to just not change anything here if there's no logical/consistent way to avoid this case. toyoshim@ is pinging the bug to ask the reporter if this was a "real" app that was broken or just a test case, and whether it's a problem to just specify different URLs.
Thank you for discussing here. Yes, I want to know if this can cause a real problem and users can not find any workaround or not. If this is not a real issue, I'd close this problem as WontFix (works as intended).
We haven't received any updates from the original reporter. So, I closed the report as WontFix because in terms of spec and implementation, this could be assumed as an intentional behavior change. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
