ocsigen / eliom

Multi-tier framework for programming web and mobile applications in OCaml.
http://eliom.org
Other
305 stars 54 forks source link

uncaught exception: Async: Eliom_request.Non_xml_content #492

Open pwbs opened 7 years ago

pwbs commented 7 years ago

e

Within this line - (eliom_request.client.ml#L276)

          Lwt.return (url, Some (result r))

the subexpression result r may raise an exception.

It's actually raised by raise Non_xml_content in line 72 but not directly, as there's an indirection: it passes by eliom_client.client.ml

Inspecting the contents of x (the x in x.XmlHttpRequest.content_xml ()), I observed that the content_xml is absent, but x.XmlHttpRequest.content contains what's expected (or almost, perhaps). However x.XmlHttpRequest.content is of type string, so it doesn't help much...

vasilisp commented 7 years ago

We are exercising this code all the time, I would guess. Is there anything special in your case? Notably, what service call does this correspond to? Is the problem reproducible on multiple browsers?

content_xml seems to come from the responseXML field of the raw JS object. Hard to say why that does not exist in your case.

pwbs commented 7 years ago

I guessed indeed that this code was exercised a lot... I kinda wished it wasn't the case. haha

In our code (I mean, in BeSport's code), it's simply coming from a

Eliom_client.change_page ~service:Eliom_service.reload_action () ()

There's likely to be something wrong with the way the navigation history is handled...

(I'm still investigating.)

pwbs commented 7 years ago

Is it reproducible on multiple browsers?

It's much easier to reproduce on Chrome (OSX). On Safari (OSX), it also happens but it's much harder to reproduce.

What I do on Chrome to reproduce it:

I go to /user/1996?tab=3 then I click to change the current column, and that rewrites part of the URL, which becomes /user/1996?tab=1 (the tab=3 mutated into tab=1). Then I do something that triggers Eliom_client.change_page ~service:Eliom_service.reload_action () () and there it fails... (if ever it doesn't fail, it reloads to tab=3 instead of tab=1, and that is what happens most often with Safari)

If I land directly on /user/1996?tab=1, it doesn't fail.

pwbs commented 7 years ago

For some reasons (that I haven't spotted yet), when it fails, the page is generated on server side (for no apparent valid reason) — whereas when it works the page is generated on client side.

vasilisp commented 7 years ago

How are you handling the tab GET parameter? As far as I can tell, we can't define a parameter type of the following form:

Eliom_parameter.(suffix (int "user") ** int "tab"))

It could be the case that the presence of ?tab=1 in the URL confuses service identification, leading to some kind of response that is not XML content.

pwbs commented 7 years ago

It's defined like that:

let user_service' =
  Eliom_service.create
    ~path:(Eliom_service.Path ["user"])
    ~meth:(Eliom_service.Get (suffix_prod (int64 "i")
                                (int "tab" ** opt (string "subtab"))))
    ()
pwbs commented 7 years ago

When it works, Eliom_route_base.find_service finds the service... whereas when it doesn't work, Eliom_route_base.find_service doesn't find the service :(

vasilisp commented 7 years ago

I can't reproduce, unfortunately. I can't help much before that happens. Would you be able to produce a small Eliom example that exhibits the issue?

pwbs commented 7 years ago

I guess that if I manage to produce a small example, I will be able to fix it... So I'm trying to understand what happens and hopefully with the very useful clues you've given me so far, I'll find a fix...

New elements... (reminder: the thing that doesn't work well is /user/1996?tab=1)

vasilisp commented 7 years ago

It may be the internal URL bookkeeping that is messed up. Reload tries to route to the current URL, so we depend on a properly-updated internal value for that (Eliom_client.current_url). To examine this, you could add a breakpoint on Eliom_client.change_url_string

pwbs commented 7 years ago

It may be the internal URL bookkeeping that is messed up. Reload tries to route to the current URL, so we depend on a properly-updated internal value for that (Eliom_client.current_url). To examine this, you could add a breakpoint on Eliom_client.change_url_string

In eliom_client.client.ml, if I replace

let change_url_string ~replace uri =

by

let change_url_string ~replace uri =
  let uri =
    match uri with
    | "./1996?tab=3" -> "user/1996?tab=3"
    | "./1996?tab=1" -> "user/1996?tab=1"
    | _ -> uri
  in

then it "works" (of course it breaks other things)... So, there's indeed something wrong related to that...

pwbs commented 7 years ago

I'm not sure what to do now, I've found a workaround, it's less than 10 bytes of difference... https://github.com/ocsigen/eliom/pull/493

I don't know if it breaks anything. I'm not sure why it "works". I'm gonna continue investigating a little...

pwbs commented 7 years ago

The workaround avoids the reload failure, but it doesn't prevent the URL to go wrong!

pwbs commented 7 years ago

I've updated https://github.com/ocsigen/eliom/pull/493 to a version that doesn't have the URL going wrong...

mfp commented 7 years ago

I've been able to reproduce the Non_xml_content issue accidentally: when the response is not valid XML, x.XmlHttpRequest.content_xml will be None, even if the XHR request was serviced correctly, and the textual contents received OK. In Chrome's Developer tools' network panel the preview will indicate the error location.

Now, how can the XML be broken? I actually triggered this in two ways:

The first one is clearly my fault. The second one is due to the response being xhtml+xml, but having a HTML doctype, I assume?

After a quick look at the code, it looks like the HTML printer does not perform attribute minimization so that HTML/XHTML compat issue should not be happening, but other might remain.

This has got nothing to do with routing, so it likely doesn't affect whatever motivated the original bug report, but the invalid XML thing might give a clue...