Closed Bug 798580 Opened 12 years ago Closed 12 years ago

b2g mochitest chromeEvent handling is broken

Categories

(Firefox OS Graveyard :: General, defect, P1)

Product:
Firefox OS Graveyard
Component:
General
Platform:
x86
Linux
Type:
defect

Tracking

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

Status:
RESOLVED FIXED
Project Flags:
blocking-basecamp +
Tracking Flags:
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: dchanm+bugzilla, Assigned: ahal)

References

Details

Attachments

(1 file)

Patch 1.0 - Run mochitests from a webapp's oop iframe
7.92 KB, patch
jgriffin
: review+
Details | Diff | Splinter Review 
b2g mochitests don't work after bug 793082. I'm not sure if this is a bug in mochitests or the current queuing system. Log below, I've removed extraneous entries


I/GeckoDump(   43): abcde shell_start
I/GeckoDump(   43): abcde sendChromeEvent
I/GeckoDump(   43): abcde undefined <value of this.isHomeLoaded>
I/GeckoDump(   43): abcde {"type":"headphones-status-changed","state":"unknown"}
I/GeckoDump(   43): abcde load
I/GeckoDump(   43): abcde sendChromeEvent
I/GeckoDump(   43): abcde true
I/GeckoDump(   43): abcde {"type":"open-app","url":"app://homescreen.gaiamobile.org/index.html","manifestURL":"app://homescreen.gaiamobile.org/manifest.webapp","isActivity":true,"target":{"filters":{"type":"application/x-application-list"},"href":"app://homescreen.gaiamobile.org/index.html"}}
----------
I/GeckoDump(  436): abcde shell_start
I/GeckoDump(  436): abcde sendChromeEvent
I/GeckoDump(  436): abcde undefined
I/GeckoDump(  436): abcde {"type":"headphones-status-changed","state":"unknown"}
I/GeckoDump(  436): abcde sendChromeEvent
I/GeckoDump(  436): abcde false
I/GeckoDump(  436): abcde {"type":"webapps-registry-ready"}
I/GeckoDump(  436): abcde webapps_observe
I/GeckoDump(  436): abcde webapps-ask-install
I/GeckoDump(  436): abcde {"app":{"installOrigin":"http://mochi.test:8888","origin":"http://example.com","manifestURL":"http://example.com/tests/webapi/apps/webapp.sjs","updateManifest":{"name":"Super Crazy Basic App","installs_allowed_from":["*"],"type":"certified","package_path":"http://example.com/tests/webapi/app.zip","permissions":{"geolocation":{"description":"geolocate"},"contacts":{"description":"contacts","access":"readwrite"}}},"etag":null,"receipts":[],"categories":[]},"from":"http://mochi.test:8888/tests/webapi/test_install.html","oid":5,"requestID":"id{09fec587-ffa4-4d46-9394-962f625fd783}","isPackage":true,"mm":{}}

Everything above the ----- works as expected.
this.isHomeLoaded is set to true after load is called [1]

At some point, start() is called again, but a subsequent load event doesn't happen, maybe because mozbrowserloadstart has already happened for the iframe? Since this.isHomeLoaded == false, future chromeEvents are not dispatched.


[1] - http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#359
This is the only place we call |shell.start()|:

http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.xul#14

I don't know how mochitests are run, but mozbrowserloadstart could be something reliable... If so we would need to find a better event that happens sooner.
Justin, is there some event other than mozbrowserloadstart that is

- the earliest event when the window object of the web content is available
- guarantee to happen before window.onload.
Blocking on broken testharness
blocking-basecamp: ? → +
Tim: This looks like your regression. Can you have a look?
Assignee: nobody → timdream+bugs
Looking into this more, the lines
I/GeckoDump(   43):
have a browserFrame with
src == app://system.gaiamobile.org/index.html
contentDocument == [object HTMLDocument]
contentWindow == [object Window]

I/GeckoDump(   436):
happens after the open-app activity for homescreen occurs
src == app://system.gaiamobile.org/
contentDocument == null
contentWindow == null

The missing contentWindow results in this line failing [1] but that appears to be a different bug. I forgot to mention in my original report that I had commented out this failing line. However that doesn't change the fact that the load event is never fired on the second go around. You can only get my above output by commenting out the failing line. The tests fail earlier if you don't comment that out.

This may not be an automation bug at all given that things are failing before navigating to the mochitest url.

[1] - http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#181
David,

I have not set up an mochitest environment yet, but testing with the device show that even with a near-empty System app index.html, mozbrowserloadstart still took place before window.onload, so my guesses in comment 2 is not correct.

However, mozbrowserloadstart will not fire if we somehow encounter an error page. |app://system.gaiamobile.org/| is not an error page (but an index page listing files) if System app is correctly installed in the profile. If gecko cannot load the page (e.g. Server not found, file not found for |app://| or |file://| urls), mozbrowserloaderror will fire instead.

Does mochitest replaces profile when test starts? If so, that where the problem is.
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #6)
> Does mochitest replaces profile when test starts? If so, that where the
> problem is.

the automation uploads a profile to /data/local/tests/profiles. This happens between the emulator startup and the running of the tests. The current workflow I see is

- emulator starts
- shell_start is called
- things are attached to window [AccessFu]
- profile / test upload happens concurrently
- profile / test upload finishes
- shell_start is called again
- things are attacked to window [AccessFu]

Both shell_start show the function caller as the onload from shell.xul, though I'm not sure where in the automation this happens.
One more thing, the open-app activity for homescreen occurs before shell_start is called the second time.
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #1)
> This is the only place we call |shell.start()|:
> 
> http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.xul#14
> 
> I don't know how mochitests are run, but mozbrowserloadstart could be
> something reliable... If so we would need to find a better event that
> happens sooner.

I see window.load there, but no mozbrowserloadstart.  window.load (and <window onload>) will /not/ fire if the iframe is remote.
> [1] - http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#181

This code is a bug and should be removed.

a) You should never touch browserFrame.contentWindow if the browserFrame may be remote, which this one may be.

b) BrowserElementChild.js already instantiates the session history on the docshell, so this code is unnecessary.
As far as I can tell, this code is extremely incorrect.  Setting aside comment 10:

http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#340

This code again assumes we can access the homescreen's content window, which again we cannot when the homescreen is remote.  Indeed, the very first line of the event handler reads the homescreen's contentWindow, which is null if the homescreen is remote.

http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#319
(In reply to Justin Lebar [:jlebar] from comment #9)
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #1)
> > This is the only place we call |shell.start()|:
> > 
> > http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.xul#14
> > 
> > I don't know how mochitests are run, but mozbrowserloadstart could be
> > something reliable... If so we would need to find a better event that
> > happens sooner.
> 
> I see window.load there, but no mozbrowserloadstart.  window.load (and
> <window onload>) will /not/ fire if the iframe is remote.

Okay, this code specifically is fine -- the onload is not on the mozbrowser window.

I filed bug 799503 on comment 10 and 11.

At this point, it does not appear that mozbrowserloadstart is unreliable -- instead, it appears that the shell.js code is extremely wrong.
(In reply to Justin Lebar [:jlebar] from comment #10)
> > [1] - http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#181
> 
> This code is a bug and should be removed.
> 
> a) You should never touch browserFrame.contentWindow if the browserFrame may
> be remote, which this one may be.
> 

I talked to :jgriffin on irc about the b2g mochitest automation. He said mochitests are run OOP, which agrees withs :jlebar's assessment. The src of the "homescreen' is redefined at [1]. I believe this means that although the system app doesn't running OOP, the resulting load of http://mochi.test:8888/ may very well be OOP.


[1] - http://mxr.mozilla.org/mozilla-central/source/build/mobile/b2gautomation.py#267
Aha.

You could test this by setting remote=false on the iframe (that is, setAttribute('remote', false);).
We do run mochitest OOP, and we do it in the "homescreen" iframe.  When we set this up months ago, cjones said this was the way to do it, since we actually didn't want Gaia apps to be loaded while mochitest was running.

If this will no longer work, we'll need to run mochitests differently.  Fabrice, can you suggest a different manner in which to load the mochitest OOP iframe?  I suppose we could replace shell.xul so it doesn't get loaded either, and then load mochitest in a content OOP iframe inside of that.
(In reply to Justin Lebar [:jlebar] from comment #14)
> Aha.
> 
> You could test this by setting remote=false on the iframe (that is,
> setAttribute('remote', false);).

Yep, I changed the browserFrame to remote=false and get the second load event now.
So what's the solution here?  Do we need to modify shell.xul to be remote-agnostic, so it works with mochitests?  Or do we need to modify how we do the mochitests?
Blocks: 780955
I guess my work is done here (by locating the right person). I am not familiar with mochitest environment so I should probably hands off from this point.

Thanks guys.
Assignee: timdream+bugs → nobody
Anyone have answers for Justin's questions in comment #17?
(In reply to Andrew Overholt [:overholt] from comment #19)
> Anyone have answers for Justin's questions in comment #17?

We need a solution here by Monday, Oct 15. We were aiming to turn on mochitests in TBPL next week, and we really hope to continue using shell.xul as we have been, but if we have to change mochitest we will. But we need to decide one way or the other because this blocking us.
Flags: needinfo?(jones.chris.g)
What are we gaining by running the mochitests in shell.xul?  It's not as though that resembles the B2G environment much, since as we see the mochitests are run OOP (also in-process, or only OOP?) and shell.xul only loads in-process content in regular B2G operation.
If shell.xul will never need to load remote content in regular operation, then we should probably change the way that we run mochitest, so we don't depend on shell.xul.
That looks the easiest thing to do. shell.xul only load the system app, and it's not running as a remote app.
shell.js is needed to help implement some DOM APIs we want to test, like wake locks.  How would the plan here affect that?
Flags: needinfo?(jones.chris.g)
(In reply to Chris Jones [:cjones] [:warhammer] from comment #24)
> shell.js is needed to help implement some DOM APIs we want to test, like
> wake locks.  How would the plan here affect that?

Apparently this would break the ability to test those.

As an alternate, could we run mochitest in a separate iframe (not 'homescreen') inside shell.xul?  Would this still allow us to test API's that are implemented in shell.js, and would it matter that Gaia's homescreen was running in the background?
P1 because bug 780955 is a P1. Who's the right assignee for this bug?
Priority: -- → P1
Not just bug 780955, though that's the only one expressed in the system. Mochitest in general on b2g is blocked by this.
(In reply to Alex Keybl [:akeybl] from comment #26)
> P1 because bug 780955 is a P1. Who's the right assignee for this bug?

I can implement the fix as soon as someone from the B2G team tells me the right way to do it.
Chris: Aren't wake locks implemented in the system app rather than in shell.xul? But I think things like touch events are implemented in shell.xul, so I think we need both.

What we want for mochitest is an environment which as much as possible looks like the environment in which webpages will be running in. Ideally that would actually mean that we would run them inside the B2G firefox browser. This actually will even affect the tests since some APIs are partially implemented by the browser app, such as window.open behavior.

But that's definitely not necessary in a first step.

What I think we should do is to have shell.xul open a mochitest app which contains a <iframe mozbrowser> (which I think by default will create a remote, OOP, window) and run the tests in there.
> What we want for mochitest is an environment which as much as possible looks like the environment in 
> which webpages will be running in.
> [...]
> What I think we should do is to have shell.xul open a mochitest app which contains a <iframe 
> mozbrowser> (which I think by default will create a remote, OOP, window) and run the tests in there.

This means that we have to modify shell.xul to work for remote apps.  I don't disagree that this is a reasonable way to proceed, but just to be clear, shell.xul does not normally ever load a remote app frame.  So this does not much resemble the normal environment.
(In reply to Justin Lebar [:jlebar] from comment #30)
> > What we want for mochitest is an environment which as much as possible looks like the environment in 
> > which webpages will be running in.
> > [...]
> > What I think we should do is to have shell.xul open a mochitest app which contains a <iframe 
> > mozbrowser> (which I think by default will create a remote, OOP, window) and run the tests in there.
> 
> This means that we have to modify shell.xul to work for remote apps.  I
> don't disagree that this is a reasonable way to proceed, but just to be
> clear, shell.xul does not normally ever load a remote app frame.  So this
> does not much resemble the normal environment.

What Jonas said does resemble the normal environment, if by normal we meant "close to Gaia frame hierarchy". shell.xul doesn't need to support OOP System app nor OOP mochitest app in this case; tests would be run within an OOP iframe contained by the in-process mochitest app (compare to OOP iframes for apps contained in the in-process System app in Gaia).

We would not be able to test the mozChromeEvent event itself in this config though. I don't see any tests testing that so I guess that's not a problem.
Nono, the whole point was that shell.xul wouldn't need to do anything special for mochitest.

My suggestion was that shell.xul would open an in-process mochitest app. This mochitest app would then run an out-of-process <iframe mozbrowser> which contains the actual frames.

That way the setup is much more similar to what we'll be doing in the shipping product. Both as far as what shell.xul needs to do, and as far as what webpages see.
That all sounds good to me.  How do I implement a mochitest app such that shell.xul knows how to open it?
(In reply to Jonathan Griffin (:jgriffin) from comment #34)
> That all sounds good to me.  How do I implement a mochitest app such that
> shell.xul knows how to open it?

Set the two perf in the profile?

user_pref("browser.manifestURL", "app://system.gaiamobile.org/manifest.webapp");
user_pref("browser.homescreenURL", "app://system.gaiamobile.org/index.html");
For these purposes, would the proper way to install the webapp be to copy the 'application.zip' and 'manifest.webapp' to /data/local/webapps and then update webapps.json?
Yes, that should work I believe.
In order to change the profile (and change prefs such as browser.manifestURL and browser.homescreenURL) we need to restart B2G which (unfortunately) also seems to re-generate webapps.json. This results in a nice little catch-22.

My understanding is that apps.install() is interactive. I feel like there is some simple way of doing this that I just don't know about.
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
Andrew, you can override browser.manifestURL and browser.homescreenURL but putting user_pref(...) in gaia/custom-prefs.js before doing |make profile| in gaia. Would that be enough for you?
No, I don't think we are able to start depending on custom builds.
What if we added a permanent generic test_harness app to gaia/test_apps which simply has an index.html with an <iframe mozbrowser>. Then the mochitests/reftests/whatever can just set that to run from shell.xul in place of the system app and load their content into the iframe.

This gets around having to create/install/register the app on the fly or needing to re-make the profile.
Fabrice, is this something we could do?
Flags: needinfo?(fabrice)
That's more or less what I asked you to test:
- add your test app to gaia/test_apps (eg mochitests-shim)
- add that to gaia/custom-prefs.js :
user_pref("browser.manifestURL", "app://mochitests-shim.gaiamobile.org/manifest.webapp");
user_pref("browser.homescreenURL", "app://mochitests-shim.gaiamobile.org/index.html");
- create the gaia profile like usual, using |make profile|
- run |b2g -profile gaia/profile|

I don't understand how you can set your test to run from shell.xul without rebuilding the profile
Flags: needinfo?(fabrice)
Right, but I was talking of checking the app into the gaia repo. It would probably be possible to rebuild the profile, but we are working within the constraints of our infrastructure (e.g releng doesn't even do gaia builds at the moment, we need to use a static copy of gaia and the emulator and install gecko into it).
Checking an app in gaia should not be a problem at all, especially when it's blocking us. We still need to figure out how to launch this app instead of the system app with your constraints. Since you're running tests on device, pushing a new user.js to /system/b2g/defaults/pref before starting the tests should be enough.
Depends on: 805585
With the patch in bug 805585 I was able to get this working with remote="false" in the app's <iframe mozbrowser>. But when I set remote="true" I get some odd errors:

http://pastebin.mozilla.org/1880780
SpecialPowers doesn't exist in the child processes.

Nice try, though.  :)
(In reply to David Chan [:dchan] from comment #0)
> At some point, start() is called again, but a subsequent load event doesn't
> happen

Actually re-reading this comment maybe the problem is simply the one in bug 805585 and none of this other stuff is necessary. David, can you try applying the patch in bug 805585 and let me know if that fixes the issue?
Flags: needinfo?(dchan+bugzilla)
(In reply to Justin Lebar [:jlebar] from comment #47)
> SpecialPowers doesn't exist in the child processes.
> 
> Nice try, though.  :)

I guess that the message manager used here doesn't work for OOP iframes?  http://mxr.mozilla.org/mozilla-central/source/testing/specialpowers/components/SpecialPowersObserver.js?force=1#32
It might work if we actually loaded SpecialPowers into the child processes.  I don't know how that's intended to work.  I just know that in my mochitests, whenever the child wants to make a specialpowers call, I have to proxy it to the parent process.
(In reply to Andrew Halberstadt [:ahal] from comment #48)
> (In reply to David Chan [:dchan] from comment #0)
> > At some point, start() is called again, but a subsequent load event doesn't
> > happen
> 
> Actually re-reading this comment maybe the problem is simply the one in bug
> 805585 and none of this other stuff is necessary. David, can you try
> applying the patch in bug 805585 and let me know if that fixes the issue?

Using the patch from 805585 didn't change the test behavior for me. isHomeLoaded wasn't set and the queued chromeEvents not fired.
Flags: needinfo?(dchan+bugzilla)
Depends on: 805829
No longer depends on: 805585
This is still blocked on bug 805829, but we can get the ball rolling while we wait.

:dchan, if you want to test it out you'll have to apply the patch in bug 805829 first as well as this one.
Attachment #677008 - Flags: review?(jgriffin)
Comment on attachment 677008 [details] [diff] [review]
Patch 1.0 - Run mochitests from a webapp's oop iframe

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

Looks good.  I see why you couldn't re-use Marionette's existing specialpowers object, because it is only defined in CONTEXT_CHROME, and this script is CONTEXT_CONTENT.
Attachment #677008 - Flags: review?(jgriffin) → review+
Heh, not sure why I thought that would work. We'll need to update the emulator again with the new gaia.
Depends on: 808674
2/3 were green. The red job is a known issue (or at least not something that could be caused by this patch).

Looks good to land
https://hg.mozilla.org/mozilla-central/rev/b5d3c03ee2b8
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [automation-needed-in-aurora]
Whiteboard: [automation-needed-in-aurora]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: