583098 - "Parsing tinderbox failed" when going back in time on MozillaTry

Status: NEW

(Core :: JavaScript Engine, defect)

Description

[Johnny Stenback (:jst)]

+++ This bug was initially created as a clone of Bug #572190 +++

Currently (and quite frequently, I think; I've seen this before), when I load http://tests.themasta.com/tinderboxpushlog/?tree=MozillaTry and click the green down arrow at the bottom to go back 12 hours, I get "Parsing Tinderbox failed. :-(" at the top, and no build data except for the top few pushes (whose builds happened in the following 12 hours).

This means that if I want to get my try server results that weren't yet ready by the time I went to sleep last night, I need to push the same thing to try again just because I can't find the results.

Comment 1

[Johnny Stenback (:jst)]

mrbkap tells me that the reason for this is that we run out of stack or JS heap space when parsing the JSON-like data that tinderbox produces. IIRC he was seeing us failing to allocate enough parser nodes in the JS parser when parsing that mass of data. This used to be more common on 64-bit systems since we had space for fewer pointers on the stack there (since the stack was the same size whether we were running on a 32 or 64 bit system), that issue was fixed in bug 572190, which I stupidly hijacked for that issue alone.

Comment 2

[Brian Crowder]

As a side note, instead of pushing new stuff to try, you could use another browser to view TBPL.  ;)

Comment 4

[Zack Weinberg (:zwol)]

As a data point, I consistently see this with FF3.6.x, but a FF4 build from last night didn't have the problem this morning.  And a no-name Webkit browser doesn't have any trouble (in fact it was enormously faster than either Firefox).

I see "script stack space quota is exhausted" appear in the error console at
the same time that the failure message appears.

Is tbpl already using JSON.parse?

Comment 5

[Johnny Stenback (:jst)]

Zack, are you on a 64-bit system? If so, the reason would probably be what was fixed in bug 572190.

As I understand it, the tinderbox JSON is not actually compliand JSON and thus won't be successfully parsable with JSON.parse() and for that reason the JS on tinderbox pushlog parses the data by loading it as a script which then runs out of space for the JS parse nodes when trying to parse it if there's enough data.

Comment 6

[Zack Weinberg (:zwol)]

(In reply to comment #5)
> Zack, are you on a 64-bit system? If so, the reason would probably be what was
> fixed in bug 572190.

Yeah.

> As I understand it, the tinderbox JSON is not actually compliand JSON and thus
> won't be successfully parsable with JSON.parse() and for that reason the JS on
> tinderbox pushlog parses the data by loading it as a script which then runs out
> of space for the JS parse nodes when trying to parse it if there's enough data.

It appears to me that the only problems with the tinderbox JSON are the leading "tinderbox_data =", trailing semicolon, and pervasive use of single- rather than double-quoted strings.  That shouldn't be hard to fix in tinderbox.

Comment 7

[Brian Crowder]

There is still a core/platform issue here, in that we cannot manage the JSON data offered by Tinderbox.  Other browsers can.  Is our stack-space still too limited?  Are we creating stack-frames other implementations are optimizing away?

Comment 8

[Robert Helmer [:rhelmer]]

(In reply to comment #6)
> It appears to me that the only problems with the tinderbox JSON are the leading
> "tinderbox_data =", trailing semicolon, and pervasive use of single- rather
> than double-quoted strings.  That shouldn't be hard to fix in tinderbox.

Bug 399190 talks about some other issues, but doesn't go into a ton of detail. Bug 399190 comment 5 says that it works ok with Crockford's JS parser.

The changes you describe would be quite easy to do, I'll mention this in bug 399190. I think this is likely to be accepted into Tinderbox.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Removing the "tinderbox_data =" is likely to break existing consumers, though.  (Some JSON APIs accept a parameter for whether to assign to a variable, and, often, what name to give it.)

Comment 10

[Robert Helmer [:rhelmer]]

(In reply to comment #9)
> Removing the "tinderbox_data =" is likely to break existing consumers, though. 
> (Some JSON APIs accept a parameter for whether to assign to a variable, and,
> often, what name to give it.)

Good point, this could be a parameter to showbuilds.cgi with the default being the current behavior. There'd need to be a different static filename though.

I'll follow up in bug 399190.

Comment 11

[Robert Helmer [:rhelmer]]

Working on getting a compliant (and smaller) JSON feed from Tinderbox that can be loaded using JSON.parse for TBPL in bug 586123.

Comment 13

[Markus Stange [:mstange]]

If somebody has this problem again, reloading and executing javascript:void(Config.goBackHours = 6) from the url bar might fix it. Then we'll only request half the time range from Tinderbox.