Page previews cannot handle empty responses gracefully - throws error in beta cluster and doesn't display preview (browser tests were failing)
Closed, ResolvedPublic2 Story Points

Description

When investigating T182465 (the failing browser tests) and proposed fix, I noticed a new issue impacting the "green-ness" of the tests.

Visit https://en.wikipedia.beta.wmflabs.org/wiki/Category:Page_Previews and hover over "Test previews no lead".

In the console this will throw an exception:

load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:52 jQuery.Deferred exception: Cannot read property 'replace' of undefined TypeError: Cannot read property 'replace' of undefined
    at Object.o [as a] (<anonymous>:124:972)
    at i (<anonymous>:127:857)
    at o (<anonymous>:127:535)
    at Object.<anonymous> (<anonymous>:126:988)
    at mightThrow (https://en.wikipedia.beta.wmflabs.org/w/load.php?debug=false&lang=en&modules=jquery%2Cmediawiki%7Cmediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:49:598)
    at process (https://en.wikipedia.beta.wmflabs.org/w/load.php?debug=false&lang=en&modules=jquery%2Cmediawiki%7Cmediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:50:269) undefined
jQuery.Deferred.exceptionHook @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:52
process @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:50
setTimeout (async)
(anonymous) @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:50
fire @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:46
fireWith @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:47
fire @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:47
fire @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:46
fireWith @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:47
done @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:126
(anonymous) @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:129
XMLHttpRequest.send (async)
send @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:130
ajax @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:125
jQuery.ajax @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:143
r @ VM77:126
c @ VM77:126
(anonymous) @ VM77:119
(anonymous) @ VM77:111
dispatch @ VM77:114
(anonymous) @ VM77:119
mightThrow @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:49
process @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:50
setTimeout (async)
(anonymous) @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:50
fire @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:46
fireWith @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:47
fire @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:47
fire @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:46
fireWith @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:47
deferred.(anonymous function) @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:51
(anonymous) @ VM77:146
setTimeout (async)
i @ VM77:146
(anonymous) @ VM77:119
(anonymous) @ VM77:111
(anonymous) @ VM77:115
(anonymous) @ VM77:130
dispatch @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:69
elemData.handle @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki|mediawiki.legacy.wikibits&only=scripts&skin=vector&version=1b46sjy:66

Looking at the code:
https://github.com/wikimedia/mediawiki-extensions-Popups/blob/master/src/gateway/rest.js#L61

it appears to only know how to handle 404s and 200 responses. It should be adjusted.

Although this error is caught and swallowed, it does not display a page preview with "no summary available".

Acceptance criteria

  • If a response completes successfully but does not have any data (e.g. 204) or does not include the extract_html field it should not throw an exception

Related Objects

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 11 2017, 10:57 PM
Jdlrobson updated the task description. (Show Details)

So RESTBase actually returns 200 and empty body for pages which MCS returns 204 for. Should we return 204 if there's no body? What's the convention here?

Originally 204 was proposed for redirect pages (which RESTBase never returns content for), so icing them as an indication of an empty summary was somewhat a surprise for me

Jdlrobson renamed this task from Page previews cannot handle 204 responses gracefully - throws error in beta cluster and doesn't display preview to Page previews cannot handle empty responses gracefully - throws error in beta cluster and doesn't display preview.Dec 11 2017, 11:45 PM

@Pchelolo I updated the title (the 200/204 is not so much an issue but the behaviour). My main concern is the JS exception that's occurring in the client, which suggests this is not being properly handled in JS. I'll defer to @phuedx about how this is actually supposed to be have, but we need to squash that JS error! :)

Jdlrobson renamed this task from Page previews cannot handle empty responses gracefully - throws error in beta cluster and doesn't display preview to Page previews cannot handle empty responses gracefully - throws error in beta cluster and doesn't display preview (browser tests failing).Dec 11 2017, 11:55 PM
ovasileva triaged this task as High priority.Dec 12 2017, 11:04 AM
ovasileva raised the priority of this task from High to Unbreak Now!.
Restricted Application added subscribers: Liuxinyu970226, Jay8g, TerraCodes. · View Herald TranscriptDec 12 2017, 12:36 PM

Additional context for T182639#3830702:

12:34:34 <phuedx> olliv: https://phabricator.wikimedia.org/T182639 is high priority if not ubn!
12:35:15 <phuedx> priority comes from the fact that the service is designed to return empty responses when things don't make sense
12:35:24 <phuedx> i think the likelihood of empty responses is low
12:35:36 <olliv> phuedx: argh, yes, saw that this morning
12:35:47 <phuedx> but it's still a broken ux
12:35:51 <phuedx> with no explanation
12:35:54 <olliv> so it's throwing an error and basically not doing anything, right?
12:35:59 <phuedx> (which i think makes it a ubn! ?)
12:36:00 <phuedx> yes
12:36:00 <olliv> yes, high, let's pull it in
12:36:09 <olliv> I'm okay with UBN

Mholloway added a comment.EditedDec 12 2017, 1:44 PM

A bug was introduced yesterday causing the 'extract' and 'extract_html' properties not to be returned for pages with empty intros. We didn't catch it because of a bug in one of the node services template test utilities wherein deepEqual(undefined, '') surprisingly evaluates to true.

As best I can tell, the errors above look like they're caused by an expected property being absent, so fixing this should fix the broken test once and for all. There might still be some client-side work around resiliency in this situation.

Change 397815 had a related patch set uploaded (by Mholloway; owner: Mholloway):
[mediawiki/services/mobileapps@master] Fix summary responses for pages with empty intros

https://gerrit.wikimedia.org/r/397815

(For further background: this response should no longer be empty after last night's fixes, but I had to purge RESTBase's cached copy this a.m. in order to reflect that.)

Change 397815 merged by jenkins-bot:
[mediawiki/services/mobileapps@master] Fix summary responses for pages with empty intros

https://gerrit.wikimedia.org/r/397815

MBinder_WMF set the point value for this task to 2.Dec 12 2017, 6:11 PM
pmiazga added a subscriber: pmiazga.EditedDec 12 2017, 6:22 PM

IMHO there is a change required in Page Previews codebase to not throw browser exception when the API response is incorrect.

Even services is going to fix the issue on their side, the frontend should be bullet-proof and not fail when API returns something that popups do not understand.

Jdlrobson lowered the priority of this task from Unbreak Now! to Normal.Dec 12 2017, 7:10 PM

Dropping to normal now as this has been addressed in beta cluster. Better error handling should not be the goal in case that ever happens again!

Jdlrobson claimed this task.
Jdlrobson renamed this task from Page previews cannot handle empty responses gracefully - throws error in beta cluster and doesn't display preview (browser tests failing) to Page previews cannot handle empty responses gracefully - throws error in beta cluster and doesn't display preview (browser tests were failing).
Jdlrobson updated the task description. (Show Details)

Change 397927 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/Popups@master] Better error handling for unexpected responses

https://gerrit.wikimedia.org/r/397927

The above patch takes care of 204s and 200s without extracts. Do we also want to cover 500 errors?

phuedx added a comment.EditedDec 13 2017, 10:23 AM

The above patch takes care of 204s and 200s without extracts. Do we also want to cover 500 errors?

Nice! And yes, I think we should cover 5xx errors.

@phuedx: as @Jdlrobson is OOO I'll follow-up on his patch.

pmiazga claimed this task.Dec 13 2017, 6:03 PM
pmiazga reassigned this task from pmiazga to bmansurov.Dec 13 2017, 6:11 PM

@bmansurov can you also add logic to handle 5xx errors and show the generic preview

bmansurov removed bmansurov as the assignee of this task.Dec 13 2017, 9:42 PM
bmansurov moved this task from Doing to Ready for Signoff on the Readers-Web-Kanbanana-Board board.
bmansurov added a subscriber: bmansurov.

Talked to @pmiazga on IRC. 5xx are already handled and the front-end won't fail when it encounters these errors. Showing the generic preview would give a false impression that a link may not have a preview, when in case the back-end maybe having a temprorary glitch.

phuedx added a subscriber: Nirzar.Dec 14 2017, 9:23 AM

^ /cc @ovasileva

IIRC we won't show a preview in the case of a backend failure (the 5xx errors we're talking about above). @Nirzar: Might it be worth adding a "Something went wrong." preview too (which is out of scope for this task BTW).

@phuedx, @Nirzar - I think it's worth adding the empty preview here too

phuedx assigned this task to ABorbaWMF.Dec 14 2017, 2:46 PM

I agree

here's an error state for previews using our blank preview design

Spacing of the icon in the mock doesn't match the existing spacing. Compare:

@bmansurov

then the existing spacing needs to be fixed.
zpl.io/bAOdAPx < this was the spec provided for it and If i remember correctly the spacing was correct? was there a change that broke the spacing?

This appears to be fixed on Beta, in that the error is now appearing. It sounds like the spacing may be a separate issue?

https://en.wikipedia.beta.wmflabs.org/wiki/Category:Page_Previews


Also now works on the PreviewsEmpty/sandbox link

Tried it on the big browsers

I'm okay showing the same empty preview for 500 errors as well

^ The above is meant to reflect that the fact that we shouldn't be deploying the Page Summary API until the major client (Page Previews) has been QA'd.

ovasileva closed this task as Resolved.Dec 15 2017, 2:14 PM

looks good. Resolving. Other issues to be tracked under T182905: Show an informative preview when the backend service responds with an error

ovasileva updated the task description. (Show Details)Dec 15 2017, 2:25 PM

Change 397927 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Better error handling for unexpected responses

https://gerrit.wikimedia.org/r/397927