894927 - Remove XUL dependency in b2g

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[[:fabrice] Fabrice Desré]

We don't really use xul in b2g (except in the <video> and <audio> bindings) but we still build everything and just using a xul window probably pages in some of this code. Let's try to get rid of what we can.

Comment 1

[[:fabrice] Fabrice Desré]

Attachment: Use xhtml instead of xul for b2g main window

That works fine for me, and the memory usage seems to go down a bit:

xhtml:

fabrice@fabrice-x230:~/dev/b2g/B2G$ adb shell b2g-info
                          |     megabytes    |
           NAME  PID NICE  USS  PSS  RSS VSIZE OOM_ADJ USER    
            b2g 1510    0 47.7 51.1 61.4 175.9       0 root    
          Usage 1564   18 10.9 14.0 24.1  65.2       6 app_1564
     Homescreen 1569   18 13.1 16.5 26.8  67.9       4 app_1569
(Preallocated a 1640   18  9.5 12.1 21.2  62.7       6 root    

System memory info:

            Total 397.2 MB
     Used - cache 100.1 MB
  B2G procs (PSS)  93.8 MB
    Non-B2G procs   6.3 MB
     Free + cache 297.2 MB
             Free 186.8 MB
            Cache 110.4 MB

=====================================================================
xul:

fabrice@fabrice-x230:~/dev/b2g/B2G$ adb shell b2g-info
                          |     megabytes    |
           NAME  PID NICE  USS  PSS  RSS VSIZE OOM_ADJ USER    
            b2g 1824    0 48.5 52.0 62.2 175.8       0 root    
          Usage 1877   18 10.9 14.0 24.1  65.2       6 app_1877
     Homescreen 1883   18 13.1 16.5 26.8  67.9       4 app_1883
(Preallocated a 1952   18  9.5 12.1 21.2  62.7       6 root    

System memory info:

            Total 397.2 MB
     Used - cache 102.1 MB
  B2G procs (PSS)  94.6 MB
    Non-B2G procs   7.5 MB
     Free + cache 295.2 MB
             Free 184.0 MB
            Cache 111.2 MB

Comment 2

[[:fabrice] Fabrice Desré]

Attachment: wip: building with --disable-xul

That doesn't compile yet... :

In file included from ../../dist/include/mozilla/dom/BindingDeclarations.h:23:0,
                 from /home/fabrice/dev/builds/obj-b2g-desktop-birch/dom/bindings/DocumentBinding.h:7,
                 from /home/fabrice/dev/builds/obj-b2g-desktop-birch/dom/bindings/HTMLAppletElementBinding.cpp:4:
../../dist/include/nsAutoPtr.h: In instantiation of ‘nsRefPtr<T>& nsRefPtr<T>::operator=(const already_AddRefed<U>&) [with I = nsFrameLoader; T = nsIFrameLoader]’:
/home/fabrice/dev/builds/obj-b2g-desktop-birch/dom/bindings/HTMLAppletElementBinding.cpp:611:33:   required from here
../../dist/include/nsAutoPtr.h:945:11: error: no matching function for call to ‘nsRefPtr<nsIFrameLoader>::assign_assuming_AddRef(nsFrameLoader* const&)’
../../dist/include/nsAutoPtr.h:945:11: note: candidate is:
../../dist/include/nsAutoPtr.h:863:7: note: void nsRefPtr<T>::assign_assuming_AddRef(T*) [with T = nsIFrameLoader]
../../dist/include/nsAutoPtr.h:863:7: note:   no known conversion for argument 1 from ‘nsFrameLoader* const’ to ‘nsIFrameLoader*’

In the directory  /home/fabrice/dev/builds/obj-b2g-desktop-birch/dom/bindings

Comment 3

[[:fabrice] Fabrice Desré]

Justin, do you think the gain from comment 1 could justify an uplift for 1.1?

Comment 4

[Justin Lebar (not reading bugmail)]

(In reply to Fabrice Desré [:fabrice] from comment #3)
> Justin, do you think the gain from comment 1 could justify an uplift for 1.1?

Probably not?  3mb (the difference in "free+cache") is a serious win for 256mb devices, but 1.1 is targeted at 512mb devices, and the 256mb devices, if they get 1.1, are incidental, aiui.

Comment 5

[[:fabrice] Fabrice Desré]

Current 1.0.1 devices will get updates to 1.1 and have only 256mb.

Comment 6

[Justin Lebar (not reading bugmail)]

(In reply to Fabrice Desré [:fabrice] from comment #5)
> Current 1.0.1 devices will get updates to 1.1 and have only 256mb.

A 3mb win is definitely significant for 256mb devices.

I don't know about the downsides of taking this patch, though.

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 777142 [details] [diff] [review]
Use xhtml instead of xul for b2g main window

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

that's probably going to break some tests http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.py#931 that use overlays.
It would be nice to check if those still works or to provide a way for them to work (this patch will also break some Gaia extensions for developing).

I also wonder if the |content| property in shell.js works if the root is not a xul document - it worth checking that to not regress.

The rest of the code looks fine to me and will definitively sign the death of xul in B2G if we can get change the video/audio bindings.

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Also can you do an about:memory to see if it is visible there?

Comment 9

[Justin Lebar (not reading bugmail)]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #8)
> Also can you do an about:memory to see if it is visible there?

about:memory is XML not XUL, but anyway we don't display it directly on the phone; we dump log files and pull them off the phone.  That shouldn't be broken by this change.

Comment 10

[[:fabrice] Fabrice Desré]

(In reply to Justin Lebar [:jlebar] from comment #9)
> (In reply to Vivien Nicolas (:vingtetun) (:21) from comment #8)
> > Also can you do an about:memory to see if it is visible there?
> 
> about:memory is XML not XUL, but anyway we don't display it directly on the
> phone; we dump log files and pull them off the phone.  That shouldn't be
> broken by this change.

I think Vivien wants to check that the memory gain we see in b2g-info also shows up in memory dumps.

Comment 11

[[:fabrice] Fabrice Desré]

Attachment: memory-logs.zip

Here are 2 about:memory logs:
about-memory-0 is without the patch, about-memory-1 with the patch. There's an improvement in the explicit allocations in the parent process.

Comment 12

[[:fabrice] Fabrice Desré]

Attachment: Use xhtml instead of xul for b2g main window, v2

This patch is green on try: https://tbpl.mozilla.org/?tree=Try&rev=cacc1cc38672

Comment 13

[Alexandre Poirot [:ochameau]]

(In reply to Fabrice Desré [:fabrice] from comment #12)
> Created attachment 778024 [details] [diff] [review]
> Use xhtml instead of xul for b2g main window, v2
> 
> This patch is green on try:
> https://tbpl.mozilla.org/?tree=Try&rev=cacc1cc38672

For some reason try build don't run b2g tests (like mochitests or xpcshell).
(I think a test everything try run them)

We might break mochitest, but may be not. The mochitest file vivien pointed out may no longer be involved for b2g testing. Both xpcshell and mochitests,
have completely different codepath for b2g:
http://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/runtestsb2g.py
http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/runtestsb2g.py
As it looks more based on marionette, which doesn't seem to use overlay,
it should'nt be impacted by this change.

Otherwise, regarding extensions, I don't think it will break gaia helper addons either as we do not do anything with shell.xul. We copy pieces of shell.js and some other JS components but that's pretty much it.
It will certainly require some work for the simulator (which uses overlays), but nothing really important as the simulator is still based on 1.1.

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 778024 [details] [diff] [review]
Use xhtml instead of xul for b2g main window, v2

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

::: b2g/chrome/content/shell.xhtml
@@ +3,5 @@
>  <!-- This Source Code Form is subject to the terms of the Mozilla Public
>     - License, v. 2.0. If a copy of the MPL was not distributed with this file,
>     - You can obtain one at http://mozilla.org/MPL/2.0/.  -->
>  
> +<xhtml xmlns="http://www.w3.org/1999/xhtml"

You mean <html, not <xhtml.

Comment 15

[[:fabrice] Fabrice Desré]

(In reply to Alexandre Poirot (:ochameau) from comment #13)

> As it looks more based on marionette, which doesn't seem to use overlay,
> it should'nt be impacted by this change.

Well, there is some test breakage to fix: https://tbpl.mozilla.org/?tree=Try&rev=ec6c501c57fb

> Otherwise, regarding extensions, I don't think it will break gaia helper
> addons either as we do not do anything with shell.xul. We copy pieces of
> shell.js and some other JS components but that's pretty much it.
> It will certainly require some work for the simulator (which uses overlays),
> but nothing really important as the simulator is still based on 1.1.

Yes, I'll sync up with Myk to make sure we have a solution for the simulator before landing anything.

(In reply to :Ms2ger from comment #14)

> 
> You mean <html, not <xhtml.

Ouch, yes. And this worked!

Comment 16

[[:fabrice] Fabrice Desré]

Attachment: Use xhtml instead of xul for b2g main window, v3

Some tests fixes later:
https://tbpl.mozilla.org/?tree=Try&rev=dcd05c668fa7

The only failure I don't understand is the change I had to make in test_element_touch.py - Without that, the test times out like if the button was not pressed.

Comment 17

[Nicholas Nethercote [inactive]]

This sounds great!

Comment 18

[[:fabrice] Fabrice Desré]

Attachment: wip: building with --disable-xul

Comment 19

[[:fabrice] Fabrice Desré]

Attachment: Use html instead of xul for b2g main window, v4

Updated patch since parts of the b2g test harness have changed. Try build is at https://tbpl.mozilla.org/?tree=Try&rev=b97ae2e3caca and looks ok.

I also have a patch that let us build with --disable-xul but I think it's out of scope for this bug. We get not much savings with it, and that breaks a few things we care about and that would need to be reimplemented (video controls, some accessibility code, the html menu element...).

I checked with the simulator guys and they are ok to land this now.

Comment 20

[[:fabrice] Fabrice Desré]

Attachment: baseline memory report

Comment 21

[[:fabrice] Fabrice Desré]

Attachment: no xul window memory report

Using the 'diff' functionnality of about:memory shows a 1.36MB win in the parent process explicit allocation with this patch.

Comment 22

[Nicholas Nethercote [inactive]]

> a 1.36MB win in the parent process explicit allocation

\o/

Comment 23

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 787821 [details] [diff] [review]
Use html instead of xul for b2g main window, v4

I know people expect me to kill xul but since this xul is my secret garden of love I prefer to leave for vacation tomorrow and I will let this honor to Alex ;)

Alex make sure the extension overlay mechanism used for both the simulator and the b2g desktop build with DEBUG=1 works.

Comment 24

[Alexandre Poirot [:ochameau]]

Comment on attachment 787821 [details] [diff] [review]
Use html instead of xul for b2g main window, v4

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

The patch looks good codewise, but we need to fix these two issues:
* on device, it looks like there is some margin or padding on top (at least),
as icons on bottoms are cropped (whereas there should be some free pixels bottom them)
* on desktop, the toplevel window ends up being a very small window like 30x30

::: build/mobile/b2gautomation.py
@@ +277,5 @@
>          # run the script that starts the tests
>          if self.test_script:
>              if os.path.isfile(self.test_script):
>                  script = open(self.test_script, 'r')
> +                print "marionnette script: ", self.test_script

Is this a debug statement or a usefull new message?

::: testing/mochitest/runtests.py
@@ +401,5 @@
>      chrome = ""
>      if options.browserChrome or options.chrome or options.a11y or options.webapprtChrome:
>        chrome += """
>  overlay chrome://browser/content/browser.xul chrome://mochikit/content/browser-test-overlay.xul
> +overlay chrome://browser/content/shell.xhtml chrome://mochikit/content/browser-test-overlay.xul

I'm not expecting this to work. TBPL doesn't use runtests.py on b2g, but runtestsb2g.py, so fixing this doesn't look mandatory.
Having said that, I'm currently submitting a patch to make this script to work on b2g desktop, but I won't consider this as a blocker to land this.

Comment 25

[[:fabrice] Fabrice Desré]

Attachment: Use html instead of xul for b2g main window, v5

(In reply to Alexandre Poirot (:ochameau) from comment #24)

> The patch looks good codewise, but we need to fix these two issues:
> * on device, it looks like there is some margin or padding on top (at least),
> as icons on bottoms are cropped (whereas there should be some free pixels
> bottom them)

I fixed that by inserting the main iframe in a <body> element.

> * on desktop, the toplevel window ends up being a very small window like
> 30x30

I fixed a mistake in screen.js, but even without this change it was ok for me on linux desktop. Which platform are you testing on?

> ::: build/mobile/b2gautomation.py
> @@ +277,5 @@
> >          # run the script that starts the tests
> >          if self.test_script:
> >              if os.path.isfile(self.test_script):
> >                  script = open(self.test_script, 'r')
> > +                print "marionnette script: ", self.test_script
> 
> Is this a debug statement or a usefull new message?

Debugging leftover, removed.

> ::: testing/mochitest/runtests.py
> @@ +401,5 @@
> >      chrome = ""
> >      if options.browserChrome or options.chrome or options.a11y or options.webapprtChrome:
> >        chrome += """
> >  overlay chrome://browser/content/browser.xul chrome://mochikit/content/browser-test-overlay.xul
> > +overlay chrome://browser/content/shell.xhtml chrome://mochikit/content/browser-test-overlay.xul
> 
> I'm not expecting this to work. TBPL doesn't use runtests.py on b2g, but
> runtestsb2g.py, so fixing this doesn't look mandatory.
> Having said that, I'm currently submitting a patch to make this script to
> work on b2g desktop, but I won't consider this as a blocker to land this.

I left my change because I'm not sure what you're asking for here, and shell.xul was obviously wrong now. But let me know if you want me eg. to just remove this line.

Comment 26

[[:fabrice] Fabrice Desré]

I re-did some memory usage measures and got that in the main process:

-0.38 MB (100.0%) -- explicit

-0.28 MB ── heap-allocated
-0.22 MB ── heap-committed
   0.80% ── heap-overhead-ratio
  24,732 ── page-faults-soft
-0.52 MB ── resident
-0.46 MB ── resident-unique
-2.05 MB ── vsize

Comment 27

[Alexandre Poirot [:ochameau]]

Comment on attachment 795841 [details] [diff] [review]
Use html instead of xul for b2g main window, v5

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

Looks good on device and desktop (linux/ubuntu).

It clearly break the simulator usage, but I don't want to block on this.
I may submit a followup to make it work with html, if required.

Comment 28

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/b2g-inbound/rev/5c25126b9c76

Comment 29

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5c25126b9c76

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Backed out for causing bug 911751.
https://hg.mozilla.org/integration/b2g-inbound/rev/3ec404b808b2

Comment 31

[Ed Morley [:emorley]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #30)
> Backed out for causing bug 911751.
> https://hg.mozilla.org/integration/b2g-inbound/rev/3ec404b808b2

Backout merged:
https://hg.mozilla.org/mozilla-central/rev/3ec404b808b2

Comment 32

[[:fabrice] Fabrice Desré]

Now with a hack for Mac OS desktop users:
https://hg.mozilla.org/integration/b2g-inbound/rev/36880bc561d6

Comment 33

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/36880bc561d6