Issue metadata
Sign in to add a comment
|
Twitter posts are not displayed in MensXP article
Reported by
ngo...@etouch.net,
Dec 7 2017
|
||||||||||||||||||||||
Issue descriptionDevice name: Pixel XL 8.0.0/(OPR3.170623.007), Nexus 5X/(N2G47O), Samsung Galaxy J2/(LMY47X), Micromax Q372/5.1 (LRX21M), Sony Xperia XA/6.0(33.2.B.2.93), ASUS_Z010D /6.0.1(MMB29P), Htc Desire 630/6.0.1(1.00.400.3), Panasonic Eluga Turbo/5.1(LMY4&D), Gionee F103/5.0 (LRX21M), Samsung Galaxy J2 5.1.1/(LMY47X) WebView version: 65.0.3287.0 Application: MensXP Application version: 1.2.4 Package name: com.til.mensxp Bisect Information: This is a regression issue broken in 'M-63' Per-Version bisect information: Good build: 63.0.3216.0 Bad build: 63.0.3217.0 Regression range: https://chromium.googlesource.com/chromium/src/+log/63.0.3216.0..63.0.3217.0?pretty=fuller&n=10000 Steps to reproduce: 1. Launch MensXP app > Go to featured section > Open the 1st article. 2. Scroll down > Observe. Expected result: Twitter posts are not displayed in the article. Actual result: Twitter posts should be displayed in the article.
,
Dec 8 2017
Per-CL bisect information: Good commit:502120 Bad commit:502121 Suspect CL: https://chromium.googlesource.com/chromium/src/+/3ac77fd0987d08feb63ca059d582cae89a30ec88 timvolodine@ Might be it looks like this issue is related to your change. please look into once, if its not related to your change please reassign back to me. Thansk!
,
Dec 8 2017
It's look like Plznavigate issue,hence adding label "Plznavigate-te". Thanks!
,
Dec 8 2017
This is WebView specific, so assigning to boliu@ for initial triage.
,
Dec 8 2017
,
Dec 8 2017
,
Dec 8 2017
,
Dec 8 2017
,
Dec 8 2017
sbashyam@ checking from MTV testing side.
,
Dec 8 2017
hmm, odd. loadDataWithBaseUrl looked to have dropped the entire data part of the load or something. Page still being reported as loaded, but opening up devtools, it's just an empty body. Here's the parameters to the loadDataWithBaseUrl java call:
base url=file:///android_asset/
data=<html><head><style>img{display: inline; height: auto !important; width: 100% !important;}iframe{display: inline; width: calc(100% - +6px) !important;}body, p, div, a, span{font-family: sans-serif-regular !important; line-height: 1.6 !important; text-align: left !important; color: #242c37 !important;}a{color: #fa3756 !important; text-decoration: none !important;}</style></head><body style="margin: 0; padding: 0"><blockquote data-instgrm-captioned="true" class="twitter-tweet" data-lang="en"><p dir="ltr" lang="en">Someone came to me and said“well ball ajay. you bowled brilliantly in last match”.played 9 years of international cricket for country and still ppl dont remember my name.😡😡<a href="https://twitter.com/hashtag/stupidity?src=hash&ref_src=twsrc%5Etfw">#stupidity</a><a href="https://twitter.com/hashtag/gavaar?src=hash&ref_src=twsrc%5Etfw">#gavaar</a></p> — Ravindrasinh jadeja (@imjadeja) <a href="https://twitter.com/imjadeja/status/938997293226717184?ref_src=twsrc%5Etfw">December 8, 2017</a></blockquote><script src="http://platform.twitter.com/widgets.js"></script></body></html>
mimeType=mText/html; charset=utf-8
encoding=UTF-8
history=null
Java still does more transformations until it reaches native code, but in case anyone can think of anything.
Odd things to call out..
file:///android_asset/ is a special url on android webview for normal loads (it shortcuts to android resources), but doesn't seem to matter for loadDataWithBaseURL
mimeType=mText/html <- what's mtext..?
,
Dec 8 2017
ahh, the bad mimetype is the issue. if I change mText to text, then everything works fine. so plznavigate is more strict with these kind of things? Is that expected? is it worth digging into exactly what dropped all the data?
,
Dec 8 2017
Thanks for tracking this down. I don't know where that's being dropped. What do you see in RenderFrameImpl::LoadDataURL? And does the net::DataURL::Parse call there change it? Do the WebView docs say that this should be a valid mimetype? It might be good to track down where this is being stripped now, in case we find this is widespread and something we have to fix.
,
Dec 8 2017
It didn't get to RenderFrameImpl::LoadDataURL. onPageStarted/Finished calls still happen though. Does that mean browser side decided renderer doesn't need to be involved with this navigation at all? Where does that logic live?
,
Dec 8 2017
I should be able to write a browser test to demonstrate this.
,
Dec 8 2017
You can try tracing through the creation of NavigationRequest & NavigationHandle; do these get the value? If there's a cross process browser test (thanks!) I can debug that too.
,
Dec 8 2017
To demonstrate problem: https://chromium-review.googlesource.com/#/c/chromium/src/+/817366 The new assert fails, ie GetLastCommittedEntry is null after, meaning the navigation just got dropped entirely? I didn't actually check if it went through in webview earlier; I guess it didn't either.
,
Dec 8 2017
I was able to repro the issue on 63 .0.3210.0 and --disable-browser-side-navigation fixes the issue way above the bad build. Tested on Pixel /OPM1.171019.014
,
Dec 8 2017
Typing "data:mtext/html; charset=utf-8,foobar" into desktop chrome browser window caused a download. So I guess we know where the navigation went..
,
Dec 8 2017
This is likely due to difference in behavior of data: URLs with PlzNavigate. In order to allow downloads, we are sending data: URLs to the network stack, where in the past they were handled fully inside the renderer process.
,
Dec 8 2017
So where does that leave this? The "mtext" thing just seems like a typo by the app developer, so probably not super widespread issue, but it's still hard to generalize. wontfix and cross our fingers? see if it's possible to get back to old behavior with plznavigate? special carve out logic just for loadDataWithBaseURL?
,
Dec 9 2017
I don't think we can revert to the old behavior, otherwise we will break data: URLs which result in downloads, which I think is a somewhat common case. Whether we can carve out logic for loadDataWithBaseURL, I don't know and will leave that to clamy@, as I am not as familiar with the intricacies of the API. It will be useful to understand if this affects any other apps, but if it is indeed just a typo, I would be supportive of WontFix.
,
Dec 9 2017
,
Dec 9 2017
Test team: we should contact this developer of this and tell them about their typo
,
Dec 11 2017
I agree with Nasko.
,
Dec 11 2017
I agree that fixing the typo is the simplest fix here. I'm going to reach out to MensXP (and open this bug so that they can see it). jamwalla@, could you update the Android API docs to clarify that we only support well-formatted mimetypes? I'm not sure if there is a list of official mimetypes...
,
Dec 12 2017
Any update here?
,
Dec 12 2017
Based on discussions so far, I'll mark this as WontFix since we won't change Chrome. It's still an open task to contact the devs.
,
Dec 12 2017
Yup. We're updating documentation in our APIs to better explain this edge case and the MensXP devs say the fix is rolled out. |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by battun@chromium.org
, Dec 7 2017Owner: battun@chromium.org
Status: Assigned (was: Unconfirmed)