1260386 - Global search in sources fails with two TypeErrors

Status: RESOLVED WONTFIX

(DevTools :: Debugger, defect, P2)

Description

[Jarda Snajdr [:jsnajdr]]

Steps to reproduce:
1. Go to www.idnes.cz (Czech news site)
2. Open debugger, and start global search, i.e., type "!something" into the searchbox.

Expected result:
The global search shows some results.

Actual result:
No search results are shown, and after a while, two unhandled promise rejections are reported in the browser console:

Full Message: TypeError:  is not iterable
Full Stack: onError@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/debugger/content/actions/sources.js:254:1

Full Message: TypeError: text is undefined
Full Stack: GlobalSearchView.prototype<._doSearch@chrome://devtools/content/debugger/views/global-search-view.js:160:11

What happens?

The page loads a document with inline scripts from a misbehaving URL: http://bbnaut.ibillboard.com/g/ca2
This URL sometimes resets the connection, sometimes returns 304 status code... the result is that loading the source from debugger throws a NS_BASE_STREAM_CLOSED, and that error is not handled correctly by the debugger code.

The issue can be reproduced only when the idnes.cz page is loaded for the first time. On repeated visits, cookies are set and different resources are loaded. I open a private window when I want to reproduce the bug.

I tried to create a minimal test case (nodejs script that serves minimal set of requests), but then I discovered bug 1259743 and had to give up. I'll try again when creating a test.

Comment 1

[Jarda Snajdr [:jsnajdr]]

Attachment: Global search in sources fails with two TypeErrors

This patch fixes two errors that cause this bug:

1. In getTextForSources(), fixed the signature of onError handler - it's called as onError(source, err), but the implemenation tries to desctructure that as onError([source, text]), leading to TypeError.

2. In loadSourceText(), a check is performed if the source is already available (cached) in the store state, and only if it isn't, an action to fetch it is dispatched. However, when the fetch fails, an {error} object is stored in the state by the reducer. When calling loadSourceText again on the same source, a promise RESOLVED with the error value is returned. That confuses the caller. Fixed that to a REJECTED promise.

There are still two problems with the code:
Big one: what if the state of the source is {isLoading: true}? Then we should wait and resolve the promise only after the source fetch is finished. I'll need some advice on how to do this with Redux. We need something like maintaining a list of unresolved promises and then resolve them in a subscription handler that listens for changes in the state.

Small one: the promise middleware throws away the Error object and stores only the message string (middleware/promise.js:45). That means that I can't return the same Error object on the second invocation as I did on the first. The API cannot idempotent, which is not very nice.

I'll need to follow up with a test, too, of course.

Comment 2

[Eddy Bruel [:ejpbruel]]

James, this one looks like it may be a regression. If so, it should be fixed asap. Could you take a look at what's going on?

Comment 3

[Eddy Bruel [:ejpbruel]]

Oops, jsnajdr pointed out that the patch he attached to the bug is intended as a fix.

Comment 4

[James Long (:jlongster)]

I'm changing this to P2 because I've fixed several P1s over the last few days and I need to work on a few other things first. I will help land it this week, but give me a day or two to think about it. With Redux, you should avoid keeping lists of promises and doing things outside the regular actions because it tends to bypass the regular pipeline and make things more confusing. There are some extraordinary circumstances which might require such things, but I'd like to think through this code. I wasn't happy with the `getTextForSources` code when migrating it so maybe there's a better way to do it.

Comment 5

[James Long (:jlongster)]

@jsnajdr are you still interested in working on this? Sorry for letting this sit. I think at this point it's clear that the new debugger is going to be where we invest most of our efforts, but I'd be happy to approve a patch that fixes this. Since will not be using this code in the new debugger, I'm not as worried about refactoring that function.

Comment 6

[James Long (:jlongster)]

Comment on attachment 8735816 [details] [diff] [review]
Global search in sources fails with two TypeErrors

Review of attachment 8735816 [details] [diff] [review]:
-----------------------------------------------------------------

I say let's land this patch to fix the error now, and if you are so inclined file a new bug about your follow-up problems.

Comment 7

[Jarda Snajdr [:jsnajdr]]

(In reply to James Long (:jlongster) from comment #5)
> @jsnajdr are you still interested in working on this?

I have a lot of other things higher on my priority list, but if a mochitest will turn out to be easy (or not needed at all), we can land the current patch.

It would be more interesting if it wasn't a frontend-only bug - the backend code will stay with us and is worth fixing.

Comment 8

[Jarda Snajdr [:jsnajdr]]

I guess this bug in the old debugger frontend is not worth landing any more.