1472136 - Detect XML parsing errors during Marionette startup

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, enhancement, P2)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I had the case today that I pulled/updated the code to latest mozilla-central, but forgot to build before trying to run some Marionette tests.

As result the YSOD (yellow screen of death) window pops-up mentioning some XML parsing errors due to changed entities. 

Problem here is that Marionette doesn't initialize correctly, and we get an IOError with "Process killed after 120s because no connection to Marionette server could be established". I think we could do better and detect such a window, and exit Firefox from within the Marionette code immediately. We could even log such a failure so it will be visible in the logs and can be picked up by Treeherder.

Given that I can always reproduce I have a solution working locally.

Maybe it would be good to also get some logging added to Firefox itself if such a case happens.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1472136 - Detect XML parsing errors during Marionette startup.

The patch lets Marionette detect a XML parser error window
during startup, which usually leaves the browser in an
unusable state. Marionette client then times out after
120s with a generic and not helpful error message.

In case such a window opens Marionette logs a fatal message
to the console, which then can be picked-up by Treeherder
for the failure summary.

Review commit: https://reviewboard.mozilla.org/r/253906/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/253906/

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Axel, do you think the checks here are sufficient or could be improved? You also mentioned a check during quit, but could this really happen?

Comment 3

[Axel Hecht [:Pike]]

looks good to me.

Comment 4

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8988698 [details]
Bug 1472136 - Detect XML parsing errors during Marionette startup.

https://reviewboard.mozilla.org/r/253906/#review260974

It would be great if we could catch this also during WebDriver:Navigate,
but that I suppose is a different problem.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8988698 [details]
Bug 1472136 - Detect XML parsing errors during Marionette startup.

https://reviewboard.mozilla.org/r/253906/#review260974

This is only chrome and can only happen during startup, or creating new chrome windows with unresolved entities. The latter is less important given that we have a running instance of Firefox and we should be able to detect this with Marionette. Note that we do not allow navigation in chrome context.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d9ba5e1359a1
Detect XML parsing errors during Marionette startup. r=ato

Comment 7

[Andreas Tolfsen ❲:ato❳]

(In reply to Henrik Skupin (:whimboo) from comment #5)

> This is only chrome and can only happen during startup, or
> creating new chrome windows with unresolved entities.

Failing to parse XHTML/XML documents is also a thing in web content.
At the moment I believe Marionette fails to return in this case.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Andreas Tolfsen 「:ato」 from comment #7)
> Failing to parse XHTML/XML documents is also a thing in web content.
> At the moment I believe Marionette fails to return in this case.

Is that different from failing due to a 404 or other error? I ask because in those cases you will have to manually check the page load status too.

Comment 9

[Andreas Tolfsen ❲:ato❳]

(In reply to Henrik Skupin (:whimboo) from comment #8)
> (In reply to Andreas Tolfsen 「:ato」 from comment #7)
> > Failing to parse XHTML/XML documents is also a thing in web content.
> > At the moment I believe Marionette fails to return in this case.
> 
> Is that different from failing due to a 404 or other error? I ask because in
> those cases you will have to manually check the page load status too.

No, I’m talking about XHTML parsing errors.

Comment 10

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/d9ba5e1359a1

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Andreas Tolfsen 「:ato」 from comment #9)
> > Is that different from failing due to a 404 or other error? I ask because in
> > those cases you will have to manually check the page load status too.
> 
> No, I’m talking about XHTML parsing errors.

Sure, my question was how those manifest. Do we silently ignore them in navigation? Because nothing like that is mentioned in the spec. But this is something completely different, and if needed should go into a separate bug. The patch here is really chrome only.

Comment 12

[Andreas Tolfsen ❲:ato❳]

(In reply to Henrik Skupin (:whimboo) from comment #11)

> (In reply to Andreas Tolfsen 「:ato」 from comment #9)  
> 
> > No, I’m talking about XHTML parsing errors.
> 
> Sure, my question was how those manifest. Do we silently ignore
> them in navigation? Because nothing like that is mentioned in the
> spec. But this is something completely different, and if needed
> should go into a separate bug.  The patch here is really chrome
> only.

Handling of invalid XHTML is implementation defined, so each driver
needs to make their own decision what to do when encountering a
parsing error.

I will file a separate bug if I can reproduce the problem.  My point
was that a patch similar to the approach here would be applicable
also in web content.