708683 - First pages loaded after startup are all gray; JavaScript Error: "JSON.parse: unexpected character" {file: "chrome://browser/content/browser.js" line: 601}

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Matt Brubeck (:mbrubeck)]

In a local build from mozilla-inbound from this morning (rev 8a49a1709b30), every page I load right after Fennec starts appears all gray, and I get repeated error console messages:

E/GeckoConsole(21976): [JavaScript Error: "JSON.parse: unexpected character" {file: "chrome://browser/content/browser.js" line: 601}]

The exception happens because the JSON in the Viewport:Change message is like this:

{ "x" : 0.0, "y" : 0.0, "width" : NaN, "height" : NaN, "pageWidth" : 600.0, "pageHeight" : 905.0, "offsetX" : 0.0, "offsetY" : 0.0, "zoom" : 0.6122449 }

NaN is not an accepted value for JSON.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

related to bug 706882 ?

Comment 2

[Kartikaya Gupta (email:[e-mail])]

On a build from https://hg.mozilla.org/mozilla-central/rev/63bff373cb94, I'm seeing this:

- Start Fennec
- logcat has a bunch of these lines:
E/GeckoConsole(18127): [JavaScript Error: "JSON.parse: unexpected character" {file: "chrome://browser/content/browser.js" line: 602}]

- If I load a page, it seems to render OK. However, if I open a new tab and load a page, it does not render, I keep getting the error line above.

Comment 3

[Aaron Train [:aaronmt]]

I get into this state after looking at [1] and playing around with pinch zooming and panning; eventually the canvas turns grey and my log is flooded with the same error.

[1] http://mozcom-cdn.mozilla.net/img/covehead/firefox/brand-toolkit/download/logo-only.png

Tested on:

20111209120720
http://hg.mozilla.org/integration/mozilla-inbound/rev/c5f491145a27
Samsung Nexus S (Android 2.3.6)

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Attachment: (1/2) Fix bad viewport values

Page size was zero initially, causing the scaleFactor to be Infinity, resulting in viewport size corruption.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Attachment: (2/2) Improve JSON-ification of viewport

This doesn't actually fix the problem since it throws a JSONException in Java if a NaN comes through, but it might guard against other kinds of badness. Also I think somebody way back when mentioned that we should be doing this.

Comment 6

[Patrick Walton (:pcwalton)]

Comment on attachment 580547 [details] [diff] [review]
(2/2) Improve JSON-ification of viewport

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

LGTM

Comment 7

[Patrick Walton (:pcwalton)]

Comment on attachment 580544 [details] [diff] [review]
(1/2) Fix bad viewport values

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

r+

Comment 8

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

mevans found an offshoot of the issue today.  
STR for another repro of this issue in a new tab:
1. go to a webpage
2. open in a new tab : http://www.html5demos.com

Comment 9

[Chris Lord [:cwiiis]]

Attachment: (3/3?) More explicitly disallow overidding of viewport size

I ran into this today and ended up 'fixing' it (but in a far less thorough way), but it might be nice to get this change in too? This disallows changing the viewport size in any way other than by setViewportSize.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Landed directly on m-c due to m-i closure.

https://hg.mozilla.org/mozilla-central/rev/4850eb9ce32a
https://hg.mozilla.org/mozilla-central/rev/73979783bac9

Leaving open since I didn't see cwiiis patch until just now

Comment 11

[Chris Lord [:cwiiis]]

Attachment: (3/3?) More explicitly disallow overidding of viewport size

First bit was left over from earlier experimentation, reattaching.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 580895 [details] [diff] [review]
(3/3?) More explicitly disallow overidding of viewport size

In general looks good, but I'm not sure why you use newViewport instead of mGeckoViewport in this hunk.

>                     public void run() {
>                         if (onlyUpdatePageSize) {
>                             // Don't adjust page size when zooming unless zoom levels are
>                             // approximately equal.
>-                            if (FloatUtils.fuzzyEquals(controller.getZoomFactor(), mGeckoViewport.getZoomFactor()))
>-                                controller.setPageSize(mGeckoViewport.getPageSize());
>+                            if (FloatUtils.fuzzyEquals(controller.getZoomFactor(), newViewport.getZoomFactor()))
>+                                controller.setPageSize(newViewport.getPageSize());
>                         } else {
>-                            controller.setViewportMetrics(mGeckoViewport);
>+                            controller.setViewportMetrics(newViewport);
>                             controller.notifyPanZoomControllerOfGeometryChange(true);

Comment 13

[Chris Lord [:cwiiis]]

(In reply to Kartikaya Gupta (:kats) from comment #12)
> Comment on attachment 580895 [details] [diff] [review]
> (3/3?) More explicitly disallow overidding of viewport size
> 
> In general looks good, but I'm not sure why you use newViewport instead of
> mGeckoViewport in this hunk.
> 
> >                     public void run() {
> >                         if (onlyUpdatePageSize) {
> >                             // Don't adjust page size when zooming unless zoom levels are
> >                             // approximately equal.
> >-                            if (FloatUtils.fuzzyEquals(controller.getZoomFactor(), mGeckoViewport.getZoomFactor()))
> >-                                controller.setPageSize(mGeckoViewport.getPageSize());
> >+                            if (FloatUtils.fuzzyEquals(controller.getZoomFactor(), newViewport.getZoomFactor()))
> >+                                controller.setPageSize(newViewport.getPageSize());
> >                         } else {
> >-                            controller.setViewportMetrics(mGeckoViewport);
> >+                            controller.setViewportMetrics(newViewport);
> >                             controller.notifyPanZoomControllerOfGeometryChange(true);

This is a race-condition - if we use mGeckoViewport in this block, there's the possibility it will be changed before we hit the block, so we'll have the viewport of the next frame before the upload that it applies to happens (I think).

Comment 14

[Kartikaya Gupta (email:[e-mail])]

(In reply to Chris Lord [:cwiiis] from comment #13)
> 
> This is a race-condition - if we use mGeckoViewport in this block, there's
> the possibility it will be changed before we hit the block, so we'll have
> the viewport of the next frame before the upload that it applies to happens
> (I think).

I'm not sure about this. If the mGeckoViewport changes before we hit that block, then the mTileLayer's origin and the graphics buffer will also be updated with the newer draw/information, and so it makes more sense to use the newer mGeckoViewport. Although I guess there is still some sort of race there, because mGeckoViewport is accessed from two threads without synchronization. I'm not really sure what to do about that short of adding more synchronization around that code.

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

Maybe you guys should spin this out into a different bug, so we can close this one as FIXED.

Comment 16

[Chris Lord [:cwiiis]]

I have a patch in bug #710119 that fixes something that can cause bad viewports (and result in this), I think once we resolve that, all(?) cases of this will be fixed.

Comment 17

[Chris Lord [:cwiiis]]

Attachment: (4/4?) Add further guards against bad viewports in browser.js

This patch adds two further guards in browser.js to stop null values from causing bad viewport updates, and restructures the ViewportMetrics toJSON function so that if it fails, the stack-trace will tell us which variable caused the failure.

Comment 18

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 581221 [details] [diff] [review]
(4/4?) Add further guards against bad viewports in browser.js

Not sure why you reversed the order of variables in toJSON() but it shouldn't make a difference. Looks fine to me.

Comment 19

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Removing qa^ as this is now a P1.

Comment 20

[Kartikaya Gupta (email:[e-mail])]

Landed patch 4, marking bug fixed.

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

Comment 21

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 580895 [details] [diff] [review]
(3/3?) More explicitly disallow overidding of viewport size

r- on this patch to get it out of my queue.

Comment 22

[Cristian Nicolae (:xti)]

Verified fixed on:

Firefox 13.0a1 (2012-02-20)
20120220031231
http://hg.mozilla.org/mozilla-central/rev/561771f01881

Firefox 12.0a2 (2012-02-20)
20120220042008
http://hg.mozilla.org/releases/mozilla-aurora/rev/dfb7c3567c49

Firefox 11.0 (2012-02-19)
20120219151859
http://hg.mozilla.org/releases/mozilla-beta/rev/6df75f4d5ccc

--
Device: Motorola Droid PRO
OS: Android 2.3.3