Closed Bug 708683 Opened 8 years ago Closed 8 years ago

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

Categories

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

ARM
Android
defect

Tracking

()

VERIFIED FIXED
Tracking Status
firefox11 --- verified
firefox12 --- verified
firefox13 --- verified
fennec 11+ ---

People

(Reporter: mbrubeck, Assigned: kats)

Details

(Keywords: regression)

Attachments

(4 files, 1 obsolete file)

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.
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.
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)
Assignee: nobody → bugmail.mozilla
Page size was zero initially, causing the scaleFactor to be Infinity, resulting in viewport size corruption.
Attachment #580544 - Flags: review?(pwalton)
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.
Attachment #580547 - Flags: review?(pwalton)
Comment on attachment 580547 [details] [diff] [review]
(2/2) Improve JSON-ification of viewport

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

LGTM
Attachment #580547 - Flags: review?(pwalton) → review+
Comment on attachment 580544 [details] [diff] [review]
(1/2) Fix bad viewport values

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

r+
Attachment #580544 - Flags: review?(pwalton) → review+
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
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.
Attachment #580894 - Flags: review?(bugmail.mozilla)
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
First bit was left over from earlier experimentation, reattaching.
Attachment #580894 - Attachment is obsolete: true
Attachment #580894 - Flags: review?(bugmail.mozilla)
Attachment #580895 - Flags: review?(bugmail.mozilla)
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);
(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).
(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.
Priority: -- → P1
Maybe you guys should spin this out into a different bug, so we can close this one as FIXED.
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.
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.
Attachment #581221 - Flags: review?(bugmail.mozilla)
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.
Attachment #581221 - Flags: review?(bugmail.mozilla) → review+
Removing qa^ as this is now a P1.
Whiteboard: [QA^]
Landed patch 4, marking bug fixed.

https://hg.mozilla.org/mozilla-central/rev/a111d03d465c
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
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.
Attachment #580895 - Flags: review?(bugmail.mozilla) → review-
tracking-fennec: --- → 11+
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.