Closed Bug 977704 Opened 8 years ago Closed 8 years ago

Back button reloads page without images

Categories

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

30 Branch
ARM
Android
defect

Tracking

(firefox29 affected, firefox30 affected, fennec32+)

RESOLVED WORKSFORME
Tracking Status
firefox29 --- affected
firefox30 --- affected
fennec 32+ ---

People

(Reporter: delza, Assigned: kbrosnan)

References

(Depends on 1 open bug)

Details

(Whiteboard: [A4A])

Attachments

(3 files)

We're seeing this a lot in Marketplace. When you surf around for awhile (still trying to get a better handle on how long for reproducibility, but not more than 3-4 pages on my Nexus 7) then use the back button to return to the Marketplace landing page, that page will reload with most or all of the images missing. Any remaining images tend to be be ones that were included in the pages you were visiting before hitting the back button.

Looking at this in developer tools (remote from desktop FF), it looks as if there is no request fired at all when returning to the page, so all content would be loaded from cache. It appears that the images are shifted out of the cache aggressively, but they are not detected as missing. This is my hypothesis only, I have no insight into how the caching is working here.

The only programmatic way I have so far of detecting that this has happened is to create an offscreen area in the DOM, iterate through the images and clone them into the offscreen area, then remove their height and width attributes. Any that then have a width property of 0 were not loaded and need to be forced to reload, either by reloading the page or by updating the src with a different cachebusting query.
Blocks: 968380
Whiteboard: [A4A]
Blocks: 972068
No longer blocks: 968380
Blocks: 969673
No longer blocks: 972068
tracking-fennec: --- → ?
tracking-fennec: ? → 29+
Assignee: nobody → wjohnston
tracking-fennec: 29+ → ?
Wes, you had a look at this before right?
Just fyi, Marketplace has its own navigation logic so ... https://github.com/mozilla/fireplace/blob/master/hearth/media/js/navigation.js
We'll need more solid STR to confirm, but snorp has suggested that this is fall out from tnikkel's work to discard offscreen images
Flags: needinfo?(delza)
Timothy said we could flip this pref: layout.imagevisibility.enable
layout.imagevisibility.enabled
Flip the pref for Firefox on Android.
Assignee: wjohnston → kbrosnan
Status: NEW → ASSIGNED
Attachment #8390839 - Flags: review?(wjohnston)
This has an extra : in the pref. I'd much prefer a real platform fix though. Can we just do this on beta/release (until we have a real fix).
(In reply to Wesley Johnston (:wesj) from comment #7)
> This has an extra : in the pref. I'd much prefer a real platform fix though.
> Can we just do this on beta/release (until we have a real fix).

I agree. I want Timothy to get a chance to see if he can debug this before we turn it off. Firefox OS also uses the pref and does not seem to have this issue. I assume it doesn't anyway. Can we get confirmation on that?

Firefox OS and Fennec do differ on some other image related prefs:
http://mxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js#301
http://mxr.mozilla.org/mozilla-central/source/mobile/android/app/mobile.js#585
Krupa and I can't reproduce it with the pref set to false. So just for completeness we went and flipped the pref back on. And we can't reproduce it with the pref set to true either anymore. 

I'm not sure what else has changed, but I'm suspecting you don't need to flip this on our behalf - unless you think there's a good reason to do this.
Closing based on the fact that we can't reproduce this anymore.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → WONTFIX
Fixed is probably more appropriate than wont fix, sorry for the noise.
Flags: needinfo?(delza)
Resolution: WONTFIX → FIXED
I hit this issue on Nightly (03-13) without toggling the setting.

ashes: a9b4f
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Note that I hit this issue upon hitting the back button from the Settings page.
Would it make sense to get this in for the beta build tonight or should we wait for more investigation?
(In reply to Andy McKay [:andym] from comment #14)
> Would it make sense to get this in for the beta build tonight or should we
> wait for more investigation?
Flags: needinfo?(mark.finkle)
(In reply to Andy McKay [:andym] from comment #14)
> Would it make sense to get this in for the beta build tonight or should we
> wait for more investigation?

As far as I can tell from this bug there is no indication that the patch even fixes the bug. And landing the patch would cause memory regressions. So until there is some indication the patch actually fixes a problem we shouldn't land it.
(In reply to Timothy Nikkel (:tn) from comment #16)
> As far as I can tell from this bug there is no indication that the patch
> even fixes the bug. And landing the patch would cause memory regressions. So
> until there is some indication the patch actually fixes a problem we
> shouldn't land it.

All our testing so far has shown that flipping the pref fixes these bugs for us, comment 9 is incorrect.
(In reply to Andy McKay [:andym] from comment #17)
> All our testing so far has shown that flipping the pref fixes these bugs for
> us, comment 9 is incorrect.

Okay, can we please be clear about that? Up until this very moment I was under the impression that I did not need to investigate anything here because flipping the pref had no effect.
ni me to investigate
Flags: needinfo?(tnikkel)
I managed to reproduce once with some debugging enabled and it looked like the images were not generating display items, this would normally happen if the images were hidden somehow, say by removing from the dom, or display none. We have a fallback that should catch any image that is asked to draw to the screen, and I didn't see this fallback being hit. Meaning that the images weren't being asked to draw once we reached the error state. Perhaps there is some interaction earlier involving the images being decoding and the page and this causes the page to think the images failed to decode or display and hide the image?
Flags: needinfo?(delza)
Comment on attachment 8390839 [details] [diff] [review]
977704-set-layout.imagevisibility.enabled-false

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

You can flip to r+ with these fixes. Just wanted to make sure you saw them. If you're busy, I can write the patch.

::: mobile/android/app/mobile.js
@@ +841,5 @@
>  // How frequently to check if we should sync home provider data.
>  pref("home.sync.checkIntervalSecs", 3600);
> +
> +// disable image visibility bug 977704
> +pref("layout.imagevisibility.enabled:", false);

There's an extra ":" character here. Also, I think you should ifdef this for beta/release builds only. You want this bug kev?
Attachment #8390839 - Flags: review?(wjohnston) → review-
(In reply to Timothy Nikkel (:tn) from comment #20)
> Meaning that the
> images weren't being asked to draw once we reached the error state. Perhaps
> there is some interaction earlier involving the images being decoding and
> the page and this causes the page to think the images failed to decode or
> display and hide the image?

So you're implying that content is perhaps doing something bad here? Andy can you look to see if Marketplace relies on image load events or something like that?

Timothy are you going to look at this some more? I don't want to just turn the image visibility stuff off, because I think it's a big win even on Android.
NI on Andy for the question above
Flags: needinfo?(amckay)
(In reply to Timothy Nikkel (:tn) from comment #20)
> I managed to reproduce once with some debugging enabled and it looked like
> the images were not generating display items, this would normally happen if
> the images were hidden somehow, say by removing from the dom, or display
> none. We have a fallback that should catch any image that is asked to draw
> to the screen, and I didn't see this fallback being hit. Meaning that the
> images weren't being asked to draw once we reached the error state. Perhaps
> there is some interaction earlier involving the images being decoding and
> the page and this causes the page to think the images failed to decode or
> display and hide the image?

When I see this issue and look at the img elements in the DOM, they are neither hidden with display:none nor removed from the DOM. They are part of the tree and picking up size from attributes, so they "display" but there is no image. If I remove the width and height then the width/height properties of the img tag revert to 0, so at some level the img knows it hasn't loaded, even though it has a URL. In the network panel, there is no traffic recorded during the page reload, so wherever the img data went, it isn't attempting to reload it when we go back to the page.
Dethe will get the needinfo, I don't know the answers to those questions.
Flags: needinfo?(amckay)
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #22)
> Timothy are you going to look at this some more? I don't want to just turn
> the image visibility stuff off, because I think it's a big win even on
> Android.

I've been trying to, but I was completely unable to reproduce the problem yesterday despite trying many many times, so my progress has been halted. (I reproduced it a couple times the day before that.) So if anyone has any tips on reproducing or a simplified testcase or anything that would be extremely helpful.
(In reply to Dethe Elza [:dethe] from comment #24)
> When I see this issue and look at the img elements in the DOM, they are
> neither hidden with display:none nor removed from the DOM. They are part of
> the tree and picking up size from attributes, so they "display" but there is
> no image. If I remove the width and height then the width/height properties
> of the img tag revert to 0, so at some level the img knows it hasn't loaded,
> even though it has a URL. In the network panel, there is no traffic recorded
> during the page reload, so wherever the img data went, it isn't attempting
> to reload it when we go back to the page.

Okay, thanks for that info, that's useful.
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #22)
> (In reply to Timothy Nikkel (:tn) from comment #20)

> So you're implying that content is perhaps doing something bad here? Andy
> can you look to see if Marketplace relies on image load events or something
> like that?

We do have an event to load the images lazily for performance. They initially have data-src for the url rather than src, but as soon as the page is loaded, and the image is scrolled into view, the src is set and the data-src is deleted. And this is working fine, the images load and all is well. It is only on returning to the page via the back button that we (sometimes) don't see the images.
Flags: needinfo?(delza)
I created a try build with a hunch I have:

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tnikkel@gmail.com-e48492bcdfd4/try-android/

Since I have been having lots of trouble reproducing I would be very appreciative if anyone could try to reproduce with this build and report their results.
tracking-fennec: ? → 29+
Flags: needinfo?(mark.finkle)
I wasn't able to reproduce it, but I'm the worst tester ever. I did hit bug 986243, but I'm not sure if thats a platform or server issue at this point.
Flags: needinfo?(krupa.mozbugs)
Flags: needinfo?(delza)
Attached image blank screen
I tried with the build provided here and I have noticed that even though the screenshots are sometimes missing, they eventually load. However, I have noticed a  blank page loading upon navigating to a screen (see attached screenshot).

I'll keep trying since the original bug was quite tricky.

I didn't see anything interesting in the logcat.
Flags: needinfo?(krupa.mozbugs)
(In reply to krupa raj[:krupa] from comment #31)
> I tried with the build provided here and I have noticed that even though the
> screenshots are sometimes missing, they eventually load. However, I have
> noticed a  blank page loading upon navigating to a screen (see attached
> screenshot).
> 
> I'll keep trying since the original bug was quite tricky.

I think I've seen those issues too. I think they are separate issues.

Thanks for testing.
I made another try build based on another hunch
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tnikkel@gmail.com-ca97a2d4e8e5/try-android/
but I'm not sure how to make progress here without a fairly reliable way to reproduce, even if it takes many tries.
Flags: needinfo?(delza)
Krupa - Can you try the build in comment 33 to see if things are better?
Flags: needinfo?(krupa.mozbugs)
Krupa, Andy - Ping?
[:mfinkle] [:tn] I tried to test build in comment 33 (just now, sorry for delay), but it doesn't appear to exist anymore. Is there a newer build or a way to re-upload that build for testing?

It is tricky to test because the problem is hard to reproduce reliably in the first place, but I'll see what I can find.
Great, thanks! What is the difference between the two?
There are two different speculative fixes, each build has one of them. I'm not sure which one might (or might not) fix the problem.
So, i tried to reproduce this issue with http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tnikkel@gmail.com-e44000fd2903/try-android/ and so far, I have been unsuccessful.

This was pretty unreliable bug to reproduce so I'm reluctant to call it fixed. I'd like to take another day to confirm if that's fine.
Flags: needinfo?(krupa.mozbugs)
ya, I have tried this a bunch of times now and I haven't been able to reproduce.
I tried with both builds and was not able to reproduce with either build. With the second build (http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tnikkel@gmail.com-a3d116bbeb80/try-android/) I did see a lot of flicker with images. With both builds, if I downloaded and launched apps it would crash nightly.
Blocks: 976550
Priority: -- → P1
I think we're at the point of no return with 29.
tn - based on the feedback, is there a patch you want to move ahead with?
tracking-fennec: 29+ → 32+
Nope. The patches were diagnostic patches to try to narrow down the issue, not landable. Also, no one has been able to reproduce this once (as far as I can tell) in weeks, so we have zero new data.
(In reply to Timothy Nikkel (:tn) from comment #45)
> Nope. The patches were diagnostic patches to try to narrow down the issue,
> not landable. Also, no one has been able to reproduce this once (as far as I
> can tell) in weeks, so we have zero new data.

Is there some way we can provide you new data, we've tested using the last couple of builds.
Not sure, reproducing is everything, if we can't reproduce we can't make progress. Making a testcase that does what the marketplace does but automatically so it's not time consuming to retry many times to reproduce?

BTW, has anyone seen this bug in any build "recently"?

I'm also working on a bunch of changes to the image visibility code. Several of them could fix a problem like this. Those parts haven't been written yet. I'll keep you updated.
I've just spent 10 minutes with nightly trying to reproduce and couldn't. So let's close, if someone can reproduce, lets re-open.
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Resolution: FIXED → WORKSFORME
If anyone is able to reproduce again I would love the chance to debug this.
Clearing needinfo.
Flags: needinfo?(tnikkel)
I'm pretty sure that this is same underlying issue as bug 1194928 (which we now understand!). Glad to finally know why this was happening.
Depends on: 1194928
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.