Bug 34262

Summary: Accept XHTML MP content type as XHTML content
Product: WebKit Reporter: jtkbugz
Component: New BugsAssignee: Laszlo Gombos <laszlo.gombos>
Status: CLOSED FIXED    
Severity: Normal CC: charles.wei, commit-queue, darin, ddkilzer, eric, hausmann, jukka.jokiniva, kenneth, koivisto, koshuin, laszlo.gombos, mjs, staikos, timothy.lu, tonikitoo, webkit.review.bot, zalan
Priority: P2 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: S60 Hardware   
OS: S60 3rd edition   
Bug Depends on:    
Bug Blocks: 34751, 35784    
Attachments:
Description Flags
Patch
none
Patch 2
laszlo.gombos: review-
Patch3 - enable the mime type
none
add xhtml-mp mime type processing capability to all WebKit ports
eric: review-, eric: commit-queue-
previous patch + test case
eric: review-
test using dumpAsText
none
test output for the failure none

Description jtkbugz 2010-01-28 06:35:17 PST
STEPS TO REPRODUCE:
1. Open the browser.
2. Type the URL "www.ovi.com" in the URL/search bar.
3. Press the "Go" button
ACTUAL RESULTS:
Progress Indiacation shows loaded and the button's state changes to "Reload". But the page content is not loaded

EXPECTED RESULTS:
Page content should load.

NOTE: The same behavior observed in ANOMALY browser
Comment 1 Simon Hausmann 2010-01-28 07:36:55 PST
I assume this is on S60. Can you reproduce this in the simulator with a backtrace?
Comment 2 Jukka Jokiniva 2010-02-09 05:01:47 PST
I tested this with N5800 & Anomaly. I works with 4.6.0, but not with 4.6.1.
Comment 3 Janne Koskinen 2010-02-19 03:28:00 PST
Ok. tested this with 4.6.2 as well. tried several https websites and none of them respond. This can be reproed on emulator. Seems that it just immediately stops loading page with SSL.
Comment 4 Janne Koskinen 2010-02-19 06:32:31 PST
Ok now found the real reason. forget https and friends nothign to do with that, blaming https was just unfortunate coincidence :)

This is issue is coming from user-agent string change https://bugs.webkit.org/show_bug.cgi?id=31961. after the change our client is now considered mobile client and server redirects to mobile page. Is the client that is wrong or the server that is wrong you'll be the judge:

GET /ui/appmanager/m/d?ar=EUROPE&co=EUROPE&la=en&te=NonNokia&_nfpb=true&_pageLabel=CATEGORY_SERVICES&_nfls=false&cdncc=FI& HTTP/1.1
User-Agent: Mozilla/5.0 (SymbianOS/9.4; U; Series60/5.0; en-GB) AppleWebKit/532.4 (KHTML, like Gecko) Anomaly/0.0.0 Safari/532.4
Accept: application/xml,application/xhtml+xml,text/html;q=0.9,text/plain;q=0.8,image/png,*/*;q=0.5
Referer: http://www.ovi.com/
Connection: Keep-Alive
Accept-Encoding: gzip
Accept-Language: en-GB,*
Host: mobile.nokia.mobi

HTTP/1.1 200 OK
Server: "Nokia"
Content-Length: 7130
Content-Type: application/vnd.wap.xhtml+xml
X-Powered-By: Servlet/2.5 JSP/2.1
Cache-Control: max-age=104
Date: Fri, 19 Feb 2010 13:08:28 GMT
Connection: keep-alive

As seen from above we don't accept vnd.wap.xhtml+xml but server insists it upon us. Also notice that for some reason we are NonNokia client on 5800 express music, so something is missing from the agent-string.

Now we don't support rendering of wap pages, or we would but we don't have it flagged on...
so the task is to have:

ENABLE_WML = 1
ENABLE_WCSS = 1

and assure it works on wap pages.

I have wireshark dumps on 4.6 vs 4.6.2 and can send them on request.
Comment 5 Laszlo Gombos 2010-02-22 06:37:57 PST
As far as I know QtWebKit does nto plan to productize WML support, so server should change.
Comment 6 Simon Hausmann 2010-02-22 09:59:21 PST
(In reply to comment #5)
> As far as I know QtWebKit does nto plan to productize WML support, so server
> should change.

Great analysis Janne!

If it's just ovi.com, then I agree and the server should be fixed. In the worst case of it affecting other sites sending us WML, too, then I think we may have to change the user agent.

Zalan, can you help us with this to get ovi.com fixed?
Comment 7 zalan 2010-02-22 10:23:50 PST
(In reply to comment #6)
> (In reply to comment #5)
> > As far as I know QtWebKit does nto plan to productize WML support, so server
> > should change.
> 
> Great analysis Janne!
> 
> If it's just ovi.com, then I agree and the server should be fixed. In the worst
> case of it affecting other sites sending us WML, too, then I think we may have
> to change the user agent.
> 
> Zalan, can you help us with this to get ovi.com fixed?
Sure, I'll forward it to some guys, they might be able to help...about the string, as seen here (http://www.zytrax.com/tech/web/mobile_ids.html), we do (or used to) include the phone model too in the UA string. I'm sure the server scans for the "Nokia" str and redirects the browser accordingly. -not saying it is right. 

Here is an example of a production UA string:
Mozilla/5.0 (SymbianOS/9.4; U; Series60/5.0 Nokia5800d-1b/20.2.014; Profile/MIDP-2.1 Configuration/CLDC-1.1 ) AppleWebKit/413 (KHTML, like Gecko) Safari/413
Comment 8 Laszlo Gombos 2010-02-22 21:40:42 PST
I would think that the "532.4" version info should be the primary importance for the server to determine the client WebKit capabilities (and not the device model number as the same device model can have different browser and WebKit versions installed).
Comment 9 Petri Ojala 2010-02-22 22:12:09 PST
I think www.ovi.com is not alone. 

I tested some web pages and at least "www.facebook.com" is not loading.
Comment 10 Petri Ojala 2010-02-23 01:11:24 PST
www.facebook.com tries to redirect to m.facebook.com. 
And its Content-Type: application/vnd.wap.xhtml+xml; charset=UTF-8
Comment 11 zalan 2010-02-23 05:30:22 PST
(In reply to comment #8)
> I would think that the "532.4" version info should be the primary importance
> for the server to determine the client WebKit capabilities (and not the device
> model number as the same device model can have different browser and WebKit
> versions installed).

In an ideal world, it would be the Accept header and not the UA string. 
In a short run, enabling WML would fix the issue, but it is far from being the optimal solution for users and for Nokia in general. Is there any way to talk to some of the major content providers and let them know about the capabilities (or actually the lack of capabilities -missing wml) of the upcoming s60 browser?
Comment 12 Janne Koskinen 2010-02-23 06:20:03 PST
(In reply to comment #8)
> I would think that the "532.4" version info should be the primary importance
> for the server to determine the client WebKit capabilities (and not the device
> model number as the same device model can have different browser and WebKit
> versions installed).

Webkit version doesn't define what features it has enabled. We could have the same build with or without WML support. Server shouldn't be worried about webkit version. It is the browser that is built upon it that will define the features that it will support.
Comment 13 Laszlo Gombos 2010-02-24 13:32:41 PST
First, the returned content does not seemed to be WML, it seems to be XHTML-MP.

application/vnd.wap.xhtml+xml is the mime type for XHTML-MP.

We do not have XHTML-MP enabled for QtWebKit at the moment. I had a failed attempt to do so on the trunk 4 month ago  - https://bugs.webkit.org/show_bug.cgi?id=30270.

I will test if turning on XHTML-MP would help. Enabling XHTML-MP is has a lot less impact that it would be to enable full WML support.
Comment 14 Petri Ojala 2010-03-02 02:08:23 PST
I did test it too. I defined ENABLE_XHTMLMP flag as enabled and now web pages are displayed correctly (ovi.com and facebook.com).

So, I think we should enable this feature for Symbian platform (at least)?
Comment 15 Petri Ojala 2010-03-03 22:21:09 PST
Created attachment 49986 [details]
Patch

Patch to turn on XHTML-MP support for Symbian platform
Comment 16 WebKit Review Bot 2010-03-03 22:24:39 PST
Attachment 49986 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 17 Petri Ojala 2010-03-03 22:29:48 PST
Created attachment 49987 [details]
Patch 2

Style issue fixed
Comment 18 Simon Hausmann 2010-03-04 01:20:17 PST
I'm not sure we should enable a feature that produces quite a few different code paths in WebKit just for Symbian.

Let me ask differently: What real world _feature_ of the XHTML-MP profile do we really _need_ need? What feature in this profile makes ovi.com "better" over a version of the same web site with "desktop" profile?
Comment 19 Laszlo Gombos 2010-03-04 21:10:40 PST
Comment on attachment 49987 [details]
Patch 2

I agree with Simon. If we end-up concluding to turn on XHTML-MP we should do it for all QtWebKit ports, not just for Symbian. Marking the patch for r-.
Comment 20 Laszlo Gombos 2010-03-04 21:26:27 PST
Created attachment 50086 [details]
Patch3 - enable the mime type

It seems that it might be enough to enable processing the "application/vnd.wap.xhtml+xml" MIME type response (it this point) no need to enable full XHTML-MP build flag. I would love to get rid of the XHTMLMP guard all together rather than adding the PLATFORM(QT) guard, but I'm afraid that will need more discussion with other ports.

Tested on Linux QtWebKit build by faking the user-agent. ovi.com and facebook.com loaded OK. 

I consider this the next step, we should continue to looking into what is the correct user-agent for Symbian.
Comment 21 zalan 2010-03-05 00:23:22 PST
> Let me ask differently: What real world _feature_ of the XHTML-MP profile do we
> really _need_ need? What feature in this profile makes ovi.com "better" over a
> version of the same web site with "desktop" profile?
Apparently nothing, as this content has not much to do with mobile profile. It's only the http header that says so. The content (.html) defines itself as xhtml in the meta tag

<meta http-equiv="content-type" content="application/xhtml+xml; charset=UTF-8"/>

So it is nothing more, than just a light/mobile version of a web service with a misconfigured http server (or whoever adds the content type).

>I'm not sure we should enable a feature that produces quite a few different code >paths in WebKit just for Symbian
Coming from the other end; what would be the benefit of enabling it for all the platforms? Would you enable WML for desktop? (i know we dont have it anymore, but let's just assume we do for a sec) It is pretty much the same in the context of targeted platforms (resource constrained devices). -mp is just yet another iteration of WML in some sense (both were initiated by OMA)

...and by grepping for the changes that mp involves, there is really only a few instances where it deviates from the baseline. nothing major.
Comment 22 Tor Arne Vestbø 2010-03-05 09:39:42 PST
Please follow the QtWebKit bug reporting guidelines when reporting bugs.

See http://trac.webkit.org/wiki/QtWebKitBugs

Specifically:

  - The 'QtWebKit' component should be used for bugs/features in the public QtWebKit API layer, not to signify that the bug is specific to the Qt port of WebKit

    http://trac.webkit.org/wiki/QtWebKitBugs#Component
Comment 23 Antonio Gomes 2010-03-05 10:16:54 PST
(In reply to comment #19)
> (From update of attachment 49987 [details])
> I agree with Simon. If we end-up concluding to turn on XHTML-MP we should do it
> for all QtWebKit ports, not just for Symbian. Marking the patch for r-.


anttik added some nice points on IRC.

<anttik> tronical: it is a feature to support legacy mobile content. it should only be enabled on mobile platforms i think
<anttik> tronical: we get mobile only if the site thinks based on the ua string that we are a mobile device
<anttik> only things it seems to do are:
<anttik> - change the noscript behavior if the document is XHTMLMP
* zalan has quit (Ping timeout: 260 seconds)
<anttik> - change marquee element behavior
<anttik> - add xhtmlmp to the accept header
<anttik> tronical: on the other hand, just treating xhtmlmp content type as xhtml (or text/html) would probably be sufficient (without actually enabling the feature)
Comment 24 Eric Seidel (no email) 2010-03-05 14:15:31 PST
Comment on attachment 50086 [details]
Patch3 - enable the mime type

Why wouldn't all ports want this?
Comment 25 Laszlo Gombos 2010-03-08 07:24:40 PST
Created attachment 50219 [details]
add xhtml-mp mime type processing capability to all WebKit ports

Make this change available for all WebKit ports, based on Eric's feedback.

This does _not_ add XHTML-MP mime type to the accept herader list; rather this just enables processing XHTML-MP mime type if a server happens to respond back with that mime type.
Comment 26 Antti Koivisto 2010-03-08 07:38:28 PST
This seems like the best solution to me. Perhaps we could also eliminate the XHTML-MP specific code completely to reduce confusion? The minor behavior changes there seem totally pointless (do they exist just to pass some operator test cases or what?)

Any objections to accepting application/vnd.wap.xhtml+xml as a valid xhtml mime type on all platforms?
Comment 27 David Kilzer (:ddkilzer) 2010-03-08 21:29:51 PST
(In reply to comment #26)
> Any objections to accepting application/vnd.wap.xhtml+xml as a valid xhtml mime
> type on all platforms?

Maciej is the correct person to ask.
Comment 28 Eric Seidel (no email) 2010-03-15 13:25:09 PDT
Comment on attachment 50219 [details]
add xhtml-mp mime type processing capability to all WebKit ports

This seems OK.  We need a layout test however.

It should be possible to write a little http test which replies with the proper mime type.
Comment 29 Laszlo Gombos 2010-03-16 20:32:48 PDT
Created attachment 50869 [details]
previous patch + test case

Added a test case per Eric's request.

The test case fails without the code change and should pass for all ports after the code change. It registers xhtmlmp file extension with the XHTML MP mime type, so that if we need more tests later the mechanism is there to do so.
Comment 30 Kenneth Rohde Christiansen 2010-03-17 18:29:16 PDT
Marciej, what is your take on this?
Comment 31 Eric Seidel (no email) 2010-03-25 00:50:28 PDT
Comment on attachment 50869 [details]
previous patch + test case

Shouldn't this be dumpAsText?

Are you sure it fails w/o this change?  I'm not sure how we would test if it's being treated as xhtml.
Comment 32 Laszlo Gombos 2010-03-26 19:11:19 PDT
Created attachment 51806 [details]
test using dumpAsText
Comment 33 Laszlo Gombos 2010-03-26 19:12:09 PDT
Created attachment 51807 [details]
test output for the failure
Comment 34 Laszlo Gombos 2010-03-26 19:21:19 PDT
Thanks Eric for the review.

If xhtml-mp mime type is not accepted, the <script> tag in the test case does not get executed, so the output of the test is an "empty rendertree" - see https://bug-34262-attachments.webkit.org/attachment.cgi?id=51807

I've changed the test case (up for review) so that if the test passes it is an output of a "dumpAsText" test, but this will not change the output of the failure, which will be the same as with the previous test case.
Comment 35 Simon Hausmann 2010-03-28 13:53:03 PDT
mjs: ping :-)
Comment 36 Eric Seidel (no email) 2010-04-01 16:54:38 PDT
Comment on attachment 51806 [details]
test using dumpAsText

LGTM.
Comment 37 Laszlo Gombos 2010-04-02 01:44:27 PDT
Comment on attachment 51806 [details]
test using dumpAsText

Thanks Eric.
Comment 38 WebKit Commit Bot 2010-04-02 02:31:15 PDT
Comment on attachment 51806 [details]
test using dumpAsText

Clearing flags on attachment: 51806

Committed r56994: <http://trac.webkit.org/changeset/56994>
Comment 39 WebKit Commit Bot 2010-04-02 02:31:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 40 George Staikos 2010-04-03 07:36:58 PDT
What I see from this appears to add "support" for the mimetype and pass the content through as XHTML.  This can't work.  XHTML-MP will cause XML parsing errors when run through the XHTML parser.  It might fix this one case of one site but that's just a coincidence as far as I can tell.  I think you want XHTML-MP enabled, which would then give you what you want - the mimetype also.
Comment 41 Eric Seidel (no email) 2010-04-04 13:17:22 PDT
I'm happy to roll this out if that's the consensus.
Comment 42 Simon Hausmann 2010-04-06 02:49:04 PDT
Revision r56994 cherry-picked into qtwebkit-2.0 with commit c99c3268f7b2f1803cf61dcaef06cad855a42313
Comment 43 George Staikos 2010-04-06 11:37:30 PDT
Um Hello???  As far as I can tell this change is wrong.  No comment?
Comment 44 Laszlo Gombos 2010-04-06 14:53:04 PDT
George, thanks for helping out with this one and sorry for the delay.

You're correct that proper XHTML-MP support is behind the XHTML-MP flag and if as port wants to support XHTML-MP proper it should turn on that flag.

I think you're also correct that some XHTML-MP will cause parsing error if not properly treated as XHTML-MP content. I do not have data on this but the assumption was that most XHTML-MP content is actually just XHTML content served with the XHTML-MP mime type. Before the patch XHTML-MP mime type content was simply ignored by WebKit, after the change WebKit at least makes an attempt to render it.

We considered turning on XHTML-MP for all QtWebKit platforms, but advertizing XHTML-MP support in the Accept header might trigger servers to send mobile XHTML-MP content also on desktop platforms. We didn't wanted to introduces fragmentation within QtWebKit platforms by turning on XHTML-MP support for only few selected platforms (e.g. Symbian). 

I believe this patch is a good compromise for most WebKit ports  - it allows ports handle XHMTL-MP properly by turning on XHTML-MP, and at the same time it makes an attempt to handle XHTML-MP content if XHTML-MP is not specifically enabled.

We can certainly make this change only for the QtWebKit port as it has been initially proposed (patch #3), but I think other WebKit ports could benefit from this change as well.
Comment 45 George Staikos 2010-04-06 15:44:05 PDT
Our experience is that is not the case.  Some sites will now start giving you XHTML-MP content that is not parseable with XHTML parsers, whereas before they would send HTML.  My guess is this will cause real-world regressions on browsers such as iPhone in particular.  It caused problems for us before.
Comment 46 Charles Wei 2010-04-06 19:30:15 PDT
This fix seems a workaround for ovi.com, which sends the document with MIME type application/vnd.wap.xhtml+xml for XHTML-MP , while the content is actually XHMTL.

If a server sends the document with MIME type as application/vnd.wap.xhtml+xml, very likely it will serve XHTML-MP specific contents.  feed it to XHTML parser without XHTML-MP enabled will lose these features,which might not be what the server side expects; and might even cause other parsing problems as George noted above. 

The best solution is to enable XHTML-MP support and fix the Ovi server.
Comment 47 Janne Koskinen 2010-04-07 06:55:20 PDT
(In reply to comment #46)
> The best solution is to enable XHTML-MP support and fix the Ovi server.

How widely this user-agent series60/symbian check has spread that is hard to tell. ovi.com and facebook.com come up as positive, maybe a lot of others. Enabling XHTML-MP support seems is one solution, changing default user-agent string another.

I've done a lot of testing using either WLAN connection or HSDPA and that doesn't give a real impression how slow it can be if you are bit off the grid. Went on train trip with my own test browser and it was unsusable to browse 'normal' pages with GSM data or even with GPRS.
Another issue with normal pages is RAM footprint that they require from the devices, plus they require a lot of scrolling to use.
Comment 48 Simon Hausmann 2010-04-07 06:59:31 PDT
(In reply to comment #45)
> Our experience is that is not the case.  Some sites will now start giving you
> XHTML-MP content that is not parseable with XHTML parsers, whereas before they
> would send HTML.  My guess is this will cause real-world regressions on
> browsers such as iPhone in particular.  It caused problems for us before.

Note that this change does not affect the accept header sent to the HTTP server, so I fail to see how this patch _changes_ what the servers out there send. How can it regress?
Comment 49 Kenneth Rohde Christiansen 2010-04-26 05:39:47 PDT
So what are we doing about this bug report? Any new insight?
Comment 50 George Staikos 2010-04-26 10:04:36 PDT
I don't have much interest in debating it further.  It's strange to send content to the wrong  parser simply due to a broken website even if it doesn't affect what's sent as the accept headers.  Seems wrong to me, but I'm going to leave it at that.
Comment 51 Eric Seidel (no email) 2010-04-26 11:08:59 PDT
I have no opinions either way.  Anyone is welcome to roll this out w/o notifying or asking me further.

webkit-patch rollout 34262
Comment 52 Laszlo Gombos 2010-04-26 11:34:01 PDT
As the author of the patch, I'd like to keep this patch in, unless a more compelling argument is raised against it. 

This patch is simply an error handling mechanism; a fallback mechanism dealing with some servers sending content that is not entirely compliant. 

Having this fallback does not justify fixing the server to send back proper content if that is possible.
Comment 53 Laszlo Gombos 2010-04-29 06:14:58 PDT
Re-closing this bug as it does not look like this patch will be rolled out or changed.
Comment 54 Timothy Lu 2010-05-13 09:16:43 PDT
Re-opened this error.  Further investigation needed.
Comment 55 Laszlo Gombos 2010-05-13 10:27:22 PDT
I talked to Timothy and conlcuded that this was re-opened by mistake. 

Re-closing it, sorry about the noise.