1107627 - browser.css is never loaded

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

browser.xul links to browser.css in chrome://browser/content/ Ours is in our skin so its never loaded.

Comment 1

[Richard Newman [:rnewman]]

Does this bug basically mean "remove browser.css and links to it from browser.xul"?

Comment 2

[:Margaret Leibovic]

Looks like the only thing is a tab restore optimization added in bug 886496. We probably want to make sure that's not necessary anymore if we're going to remove the file.

Comment 3

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

Using the | display: none; | for zombie tabs saves a significant amount of memory. If we remove the CSS file, we still need to make sure this optimization is somehow used.

I figure it's just easier to keep the file.

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Patch

Fix

Comment 5

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

https://hg.mozilla.org/integration/fx-team/rev/078591b255a0

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Backed out for bringing back bug 941746 on Android 4.0/4.2.
https://hg.mozilla.org/integration/fx-team/rev/cddde1da19ae

Comment 7

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

Attachment: pending-browser v0.1

This patch also gets the CSS active again, but treats the CSS file as non-theme, like desktop.

This patch does not fix the Session OOM test failures though. I need some Brian time to work on that.

Note: This patch saves 104KB per zombie tab. Verified using about:memory.

Comment 8

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #7)
> Created attachment 8543687 [details] [diff] [review]
> pending-browser v0.1
> 
> This patch also gets the CSS active again, but treats the CSS file as
> non-theme, like desktop.

Nice, that bothered me before, but I didn't feel like sticking in my nose in to object :)
 
> This patch does not fix the Session OOM test failures though. I need some
> Brian time to work on that.

Can you reproduce locally?

Comment 9

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

Try build showing the failure:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ea3692764a68

I wonder if the test is hoping for a living document in the zombie/restored tabs?

Comment 10

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

(In reply to :Margaret Leibovic from comment #8)
 
> > This patch does not fix the Session OOM test failures though. I need some
> > Brian time to work on that.
> 
> Can you reproduce locally?

Not exactly. Locally, the test hangs after loading the restored session. Need to add some logging.

Comment 11

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

Just an update: The test failures were valid in that I found a bug in the behavior when switching to a zombie tab. Something like:

1. Restore a few tabs. The selected tab is ready (not "pending") but the other tabs are zombies ("pending") so the PresShells are not yet created. That's what | browser[pending] { display: none; } | does.
2. Switch to a different tab. Since it's a zombie it needs to be restored, which means:
  A. Remove "pending" attribute (starts the creation of the PresShell)
  B. Restore the session history, including loading content from the network or cache.
  C. Update the viewport and zoom state, which touches PresShell via DOMWindowUtils and send updates to Java.

The failure occurs in #2C because the PresShell is not fully created by the time we try to use it and we get XPCOM errors. I "fixed" that by forcing #2C to wait until the "TabSelect" event has fully played out by dispatching the calls into the threads queue. The errors go away and testSessionOOMRestore PASSES, but in at least one case, no content is rendered to the screen. This happens in testSessionOOMRestore and in real hands-on usage.

I need to find out what's causing the rendering problem. Doesn't matter that the tests are passing now if you can't view content :)

Comment 12

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

Comment on attachment 8543687 [details] [diff] [review]
pending-browser v0.1

This patch has too many side effects. I might just remove the existing code, which is never called anyway, rather than leave it in Fennec, broken.

Comment 13

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.