747883 - Bottom part of this page is not painted (usage of document.write on a timer)

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, after 500ms links are written, using document.write, from 0 to 199.
Scroll down on the page.

On the Samsung Galaxy Nexus, when scrolling down, the rendering stops at link nr. 43.
Normally rendering should continue until link nr. 199.

This might be a regression, let me know if I need to find out.

Comment 1

[Ali Juma [:ajuma]]

(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #0)
> This might be a regression, let me know if I need to find out.

Yes, it would definitely be helpful to know if this is a regression.

Comment 2

[Martijn Wargers (dead)]

I think this regressed with the landing of Maple.
It works all right with the 2012-03-14 build, I can't scroll at all in a 2012-03-15 build and later builds exhibit current behavior (I tried the 2012-04-01 build for example).

Comment 3

[Ali Juma [:ajuma]]

This sounds like the sort of thing that might have been fixed by Bug 748048, so please check if this is still happening.

Comment 4

[Adrian Tamas (:AdrianT)]

This is still an issue on Nightly 15.0a1 2012-04-07 on HTC Desire (Android 2.2). The last link I see is 57. I will do a regression range check on Maple for the issue.

Comment 5

[Adrian Tamas (:AdrianT)]

Regression on Maple builds:

March 12th build e88141f7f64c - Viewport issue. All lines can be seen after scroll but with a lot of checkerboarding.
March 12th build e3798e906917 - fail. Blocked at line 12, text is oversized and viewport is fixed.

Possible regression range:
http://hg.mozilla.org/projects/maple/pushloghtml?fromchange=e88141f7f64c&tochange=e3798e906917

Comment 6

[Kartikaya Gupta [:kats]]

:ajuma, see https://bugzilla.mozilla.org/show_bug.cgi?id=748509#c5 - what you showed me today might be caused by the same underlying issue. The symptoms are very similar.

Comment 7

[Ali Juma [:ajuma]]

(In reply to Kartikaya Gupta (:kats) from comment #6)
> :ajuma, see https://bugzilla.mozilla.org/show_bug.cgi?id=748509#c5 - what
> you showed me today might be caused by the same underlying issue. The
> symptoms are very similar.

They're indeed related, in that the problem is that isBrowserContentDocumentDisplayed is returning false, preventing the viewport from being updated. But the patch for that bug doesn't fix this one. Instead, changing the line 

> return tab.ContentDocumentIsDisplayed;

to

> return true;

fixes this, so something is wrong with the way tab.ContentDocumentIsDisplayed is being updated.

Comment 8

[Kartikaya Gupta [:kats]]

Does the before-first-paint event get fired on the page?

Comment 9

[Ali Juma [:ajuma]]

(In reply to Kartikaya Gupta (:kats) from comment #8)
> Does the before-first-paint event get fired on the page?

Yes, but then onLocationChange fires and sets this.contentDocumentIsDisplayed to false again.

Comment 10

[Kartikaya Gupta [:kats]]

Do you know why? Does the URL actually change?

Comment 11

[Ali Juma [:ajuma]]

(In reply to Kartikaya Gupta (:kats) from comment #10)
> Do you know why? Does the URL actually change?

The URL changes from
https://bugzilla.mozilla.org/attachment.cgi?id=617448
to
wyciwyg://6/https://bugzilla.mozilla.org/attachment.cgi?id=617448

Comment 12

[Kartikaya Gupta [:kats]]

Oh, that's not good. See https://bugzilla.mozilla.org/show_bug.cgi?id=434573#c2 for why that happens. We'll need to hack around it I guess. Feel free to reassign this to me if you have other more-graphics-y things to do. Otherwise, I'd just go with the simplest fix - check for this url scheme in the onLocationChange handler and don't set contentDocumentIsDisplayed back to false. Although if that url shows up in the awesomebar and is user-visible we should probably have a better fix.

Comment 13

[Ali Juma [:ajuma]]

(In reply to Kartikaya Gupta (:kats) from comment #12)
> I'd just go with the
> simplest fix - check for this url scheme in the onLocationChange handler and
> don't set contentDocumentIsDisplayed back to false.

We need to do a bit more than this, since otherwise when we have a real location change where the destination starts with wycywig, we'll get fooled: for example, we visit the test case for this bug, then go to cnn.com, then go back. So in addition to checking for the prefix, we need to store the current url; then when we get onLocationChange, if the new url has a wycywig prefix and matches the current url, we shouldn't set contentDocumentIsDisplayed back to false.

> Although if that url
> shows up in the awesomebar and is user-visible we should probably have a
> better fix.

Preventing wycywig from showing up in the awesomebar is easy to do, since we already have code in XUL Fennec that uses nsIURIFixup to do this, so we just need to bring this code over. And this makes the first part easier too, since instead of explicitly checking for a wycywig prefix, we can check if the non-fixed-up URI matches the fixed-up URI.

Comment 14

[Ali Juma [:ajuma]]

Attachment: Make onLocationChanged behave correctly for wyciwyg-prefixed URIs

This brings over XUL fennec's code for fixing up wyciwyg-prefixed URIs, and adds the logic discussed in Comment 13 for detecting whether a wyciwyg-prefixed URI in onLocationChanged means that the location hasn't really changed.

Comment 15

[Kartikaya Gupta [:kats]]

Comment on attachment 626836 [details] [diff] [review]
Make onLocationChanged behave correctly for wyciwyg-prefixed URIs

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

This looks fine to me. mfinkle should also review; I'm not sure if there are other ramifications of this behaviour.

Comment 16

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

Comment on attachment 626836 [details] [diff] [review]
Make onLocationChanged behave correctly for wyciwyg-prefixed URIs


>+  this.currentURI = "";

<browser> already has a field we can use for this: browser.lastURI

>   onLocationChange: function(aWebProgress, aRequest, aLocationURI, aFlags) {

>     let uri = aLocationURI.spec;

Let's delete this one and just use aLocation

>+    let fixedURI = aLocationURI.spec;

Let's keep things as nsIURI, not strings... so drop the ".spec"

>+    try {
>+      fixedURI = URIFixup.createExposableURI(aLocationURI).spec;

Drop the ".spec"

>+    let sameDocument = (aFlags & Ci.nsIWebProgressListener.LOCATION_CHANGE_SAME_DOCUMENT) != 0 ||
>+                       ((fixedURI != uri) && (fixedURI == this.currentURI));

Do we care if (fixedURI != uri) ? Isn't (fixedURI == this.currentURI) enough? The fact that we need this logic makes me sad. I am disappointed that the platform doesn't already give us a way to handle this.

That said, ... since uri -> aLocation and all of these are nsIURI, and I love single lines, it becomes:

      let sameDocument = (aFlags & Ci.nsIWebProgressListener.LOCATION_CHANGE_SAME_DOCUMENT) != 0 || ((!fixedURI.equals(aLocation)) && (fixedURI.equals(this.browser.lastURI)));

>+    this.currentURI = fixedURI;

      this.browser.lastURI = fixedURI;

>+        uri: fixedURI,

          uri: fixedURI.spec,

(yes, I want to avoid creating yet another URI variable)

Comment 17

[Ali Juma [:ajuma]]

(In reply to Mark Finkle (:mfinkle) from comment #16)
> Comment on attachment 626836 [details] [diff] [review]
>
> Do we care if (fixedURI != uri) ? Isn't (fixedURI == this.currentURI)
> enough?

Yes, that's enough.

Comment 18

[Ali Juma [:ajuma]]

Attachment: Make onLocationChanged behave correctly for wyciwyg-prefixed URIs

Patch updated to address review comments. Carrying forward r=mfinkle,kats.

Comment 19

[JP Rosevear [:jpr]]

Can we land this on m-c and nom for aurora?

Comment 20

[Ali Juma [:ajuma]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1880b36386f1

Comment 21

[Ali Juma [:ajuma]]

Comment on attachment 627754 [details] [diff] [review]
Make onLocationChanged behave correctly for wyciwyg-prefixed URIs

[Approval Request Comment]
User impact if declined: Pages that use document.write on a timer won't repaint in response to panning/zooming.
Testing completed (on m-c, etc.): Currently on inbound.
Risk to taking this patch (and alternatives if risky): Low-risk, mobile-only.
String or UUID changes made by this patch: None.

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1880b36386f1

Comment 23

[Ali Juma [:ajuma]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/410ad3113243

Comment 25

[Andreea Pod]

Verified fixed

Build: Firefox 16.0a1 (2012-07-06)
Device: LG Optimus 2X (Android 2.2)