Closed Bug 1472136 Opened 2 years ago Closed 2 years ago

Detect XML parsing errors during Marionette startup

Categories

(Testing :: Marionette, enhancement, P2)

enhancement

Tracking

(firefox63 fixed)

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

Details

Attachments

(1 file)

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.
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?
Flags: needinfo?(l10n)
looks good to me.
Flags: needinfo?(l10n)
Attachment #8988698 - Flags: review?(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.
Attachment #8988698 - Flags: review?(ato) → review+
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.
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d9ba5e1359a1
Detect XML parsing errors during Marionette startup. r=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.
(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.
(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.
https://hg.mozilla.org/mozilla-central/rev/d9ba5e1359a1
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
(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.
(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.
You need to log in before you can comment on or make changes to this bug.