902165 - System JS : ERROR resource://gre/modules/BrowserElementParent.jsm:219 ReferenceError: DOMApplicationRegistry is not defined in green B2G mochitest-3 runs

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

Justin, is this something you can look into? Not sure how severe it is.

https://tbpl.mozilla.org/php/getParsedLog.php?id=26218860&tree=Fx-Team#error13

b2g_emulator_vm fx-team opt test mochitest-3 on 2013-08-06 09:25:37 PDT for push 65220b0f2b68
slave: tst-linux64-ec2-308

09:57:49     INFO -  38684 INFO TEST-PASS | /tests/dom/apps/tests/test_app_update.html | Got download progress
09:57:49     INFO -  38685 INFO TEST-PASS | /tests/dom/apps/tests/test_app_update.html | Got download progress
09:57:49     INFO -  38686 INFO TEST-PASS | /tests/dom/apps/tests/test_app_update.html | App is installed
09:57:49     INFO -  38687 INFO TEST-PASS | /tests/dom/apps/tests/test_app_update.html | Checking installed app
09:57:49     INFO -  System JS : ERROR resource://gre/modules/BrowserElementParent.jsm:219
09:57:49     INFO -                       ReferenceError: DOMApplicationRegistry is not defined
09:57:49     INFO -  ###################################### forms.js loaded
09:57:49     INFO -  ############################### browserElementPanning.js loaded
09:57:49     INFO -  ######################## BrowserElementChildPreload.js loaded
09:57:49     INFO -  38688 INFO TEST-PASS | /tests/dom/apps/tests/test_app_update.html | Message from app: OK: Launched app
09:57:49     INFO -  38689 INFO TEST-PASS | /tests/dom/apps/tests/test_app_update.html | App is installed
09:57:49     INFO -  38690 INFO TEST-PASS | /tests/dom/apps/tests/test_app_update.html | Message from app: OK: Really Rapid Release (cached) == Really Rapid Release (cached) - Manifest name should be correct
09:57:49     INFO -  38691 INFO TEST-PASS | /tests/dom/apps/tests/test_app_update.html | Message from app: OK: http://test == http://test - App origin should be correct
09:57:49     INFO -  38692 INFO TEST-PASS | /tests/dom/apps/tests/test_app_update.html | Message from app: OK: http://mochi.test:8888 == http://mochi.test:8888 - Install origin should be correct
09:57:49     INFO -  38693 INFO TEST-PASS | /tests/dom/apps/tests/test_app_update.html | Version should be correct
09:57:49     INFO -  38694 INFO TEST-PASS | /tests/dom/apps/tests/test_app_update.html | Messaging from app complete
09:57:49     INFO -  38695 INFO TEST-PASS | /tests/dom/apps/tests/test_app_update.html | Setting callbacks
09:57:49     INFO -  38696 INFO TEST-PASS | /tests/dom/apps/tests/test_app_update.html | Checking for updates
09:57:49     INFO -  38697 INFO TEST-PASS | /tests/dom/apps/tests/test_app_update.html | downloadapplied fired.
09:57:49     INFO -  38698 INFO TEST-KNOWN-FAIL | /tests/dom/apps/tests/test_app_update.html | lastUpdateCheck updated appropriately
09:57:49     INFO -  -- webapps.js uninstall http://test/tests/dom/apps/tests/file_app.sjs?apptype=cached&getmanifest=true
09:57:49     INFO -  38699 INFO TEST-PASS | /tests/dom/apps/tests/test_app_update.html | Uninstalled app
09:57:49     INFO -  38700 INFO TEST-PASS | /tests/dom/apps/tests/test_app_update.html | Checking uninstalled app
09:57:49     INFO -  ************************************************************
09:57:49     INFO -  * Call to xpconnect wrapped JSObject produced this error:  *
09:57:49     INFO -  [Exception... "'[JavaScript Error: "DOMApplicationRegistry is not defined" {file: "resource://gre/modules/BrowserElementParent.jsm" line: 219}]' when calling method: [nsIObserver::observe]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: http://mochi.test:8888/tests/dom/apps/tests/test_app_update.html :: checkAppState :: line 249"  data: yes]
09:57:49     INFO -  ************************************************************
09:57:49     INFO -  ###################################### forms.js loaded
09:57:49     INFO -  ############################### browserElementPanning.js loaded
09:57:49     INFO -  ######################## BrowserElementChildPreload.js loaded

Comment 1

[Justin Lebar (not reading bugmail)]

This is a bit odd; it looks like this means that importing Webapps.jsm didn't create the DOMApplicationRegistry object?  Maybe we hit an exception loading Webapps.jsm and it's being swallowed?  Fabrice might know more about this than me...

Comment 2

[[:fabrice] Fabrice Desré]

Is this intermittent? Indeed it looks like we fail either to load Webapps.jsm at all (the exception could come from one of the other jsm it imports), or DOMApplicationRegistry.init() throws. I've never hit that before.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

I'm seeing it in every random log I've looked at. We're only noticing it now due to recent TBPL log parser improvements that landed (bug 892958).

Comment 4

[Justin Lebar (not reading bugmail)]

Kyle, is it possible that our manual b2g compartment merging is somehow getting us into trouble here?  BrowserElementParent.jsm does

> XPCOMUtils.defineLazyGetter(this, "DOMApplicationRegistry", function () {
>   Cu.import("resource://gre/modules/Webapps.jsm");
>   return DOMApplicationRegistry;
> });

and then it references DOMApplicationRegistry in the BEP constructor...

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I hesitate to say it's impossible, but it seems unlikely.  I would not expect problems caused by compartment sharing to be intermittent either.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

This isn't intermittent. It happens on every run. It just currently isn't a fatal exception.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

2:25:27 PM - RyanVM|Sheriff: khuey: any more thoughts on bug 902165 (DOMApplicationRegistry not defined)
2:27:28 PM - khuey: RyanVM|Sheriff: it's possible, yes

Back to you, jlebar! :)

Comment 8

[Justin Lebar (not reading bugmail)]

I don't think I'm going to get to this before I leave on Friday.  Sorry.  :(

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Back on to khuey's ever-growing heap then, I guess :-\

Comment 10

[[:fabrice] Fabrice Desré]

Kyle, I don't think we gain much by lazy loading the Webapps.jsm in the parent, since it will be loaded anyway quite early by shell.js. So if moving to a simple Cu.import() fixes this bug, r=me

Comment 11

[[:fabrice] Fabrice Desré]

Attachment: patch

Comment 12

[Justin Lebar (not reading bugmail)]

Comment on attachment 797894 [details] [diff] [review]
patch

bizarre, but if it works, ship it!

Comment 13

[[:fabrice] Fabrice Desré]

That helps with the M3 errors, but that now triggers loading ActivitiesService.jsm in child processes, which doesn't end well. I have no idea yet how this can happen tough :(

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Sorry to bug you Fabrice, but any news to report? :)

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://tbpl.mozilla.org/php/getParsedLog.php?id=28495674&tree=Mozilla-Inbound#error0

Comment 16

[Reuben Morais [:reuben]]

https://tbpl.mozilla.org/php/getParsedLog.php?id=28500283&tree=Mozilla-Inbound#error0

Comment 17

[[:fabrice] Fabrice Desré]

Attachment: bep-in-parent-only.patch

I finally found the root cause... BrowserElementParent.js creation was triggered by being in the app-startup category, but we create these components in both the parent and child processes. This is actually what you want sometimes, but definitely not in this case: the code ended up trying to load and run a bunch of code in the child that is parent only. The test failure was happening when trying to create a parent process manager in the child.

The approach in this patch is to create a new category, named 'app-startup-parent'. Components that only need to run in the parent register themselves to this category.

try run at:
https://tbpl.mozilla.org/?tree=Try&rev=309f671b0cda

Comment 18

[[:fabrice] Fabrice Desré]

Comment on attachment 8339505 [details] [diff] [review]
bep-in-parent-only.patch

Tests don't look happy.

Comment 19

[[:fabrice] Fabrice Desré]

Attachment: rebased patch

Moving that patch in the right bug.

Try run at https://tbpl.mozilla.org/?tree=Try&rev=adbc5e34ff11

Comment 20

[[:fabrice] Fabrice Desré]

Comment on attachment 8395059 [details] [diff] [review]
rebased patch

try doesn't seem happy...

Comment 21

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8395059 [details] [diff] [review]
rebased patch

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

Let me know when you have a patch that passes try :)

Comment 22

[Marco Castelluccio [:marco]]

Attachment: Patch

Hey Fabrice, there was a missing parenthesis in your patch. I've added it and added a break in the RegisterBEP case.

The patch looks green on try (so far): https://tbpl.mozilla.org/?tree=Try&rev=d7ba1d2a1577

Comment 23

[[:fabrice] Fabrice Desré]

/me facepalms

Comment 24

[(no longer active)]

(In reply to comment #23)
> /me facepalms

Blame JS! :)

Comment 25

[Marco Castelluccio [:marco]]

(In reply to Fabrice Desré [:fabrice] from comment #23)
> /me facepalms

:D

Do you want to re-request review to bent? (or maybe someone else since he's on vacation)

Comment 26

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8427342 [details] [diff] [review]
Patch

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

r=me with this fixed:

::: dom/apps/src/Webapps.jsm
@@ +1150,5 @@
>          break;
>        case "Webapps:ReplaceReceipt":
>          this.replaceReceipt(msg, mm);
>          break;
> +      case "Webapps:RegisterBEP":

I think we should make sure the app has the 'browser' permission before we allow this.

If I'm confused and we always have one even for apps without this permission then we could at least make sure that it only does this once I think.

Comment 27

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0002-Bug-902165-System-JS-ERROR-resource-gre-modules-Brow.patch

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #26)

> I think we should make sure the app has the 'browser' permission before we
> allow this.
> 
> If I'm confused and we always have one even for apps without this permission
> then we could at least make sure that it only does this once I think.

TBH I'm not completely sure what that second comment means, but I did make a change to Fabrice's patch that checks for the "browser" permission, and the tests seem to pass - https://tbpl.mozilla.org/?tree=Try&rev=e9775a1f53c5.  Fabrice, does this patch take Ben's comments into account?

Comment 28

[[:fabrice] Fabrice Desré]

Comment on attachment 8493442 [details] [diff] [review]
0002-Bug-902165-System-JS-ERROR-resource-gre-modules-Brow.patch

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

::: dom/apps/Webapps.jsm
@@ +4294,5 @@
> +      return;
> +    }
> +    if (!app.hasPermission("browser")) {
> +      return;
> +    }

That's not how we test permissions in ipc code. You need to use assertPermission)() like in http://mxr.mozilla.org/mozilla-central/source/dom/apps/Webapps.jsm?rev=5f06a3b5fd42#1167

Comment 29

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0002-Bug-902165-System-JS-ERROR-resource-gre-modules-Brow.patch

(In reply to Fabrice Desré [:fabrice] from comment #28)
> That's not how we test permissions in ipc code. You need to use
> assertPermission)() like in
> http://mxr.mozilla.org/mozilla-central/source/dom/apps/Webapps.
> jsm?rev=5f06a3b5fd42#1167

Like this?  Try at https://tbpl.mozilla.org/?tree=Try&rev=a606605886f6

Comment 30

[[:fabrice] Fabrice Desré]

Comment on attachment 8494256 [details] [diff] [review]
0002-Bug-902165-System-JS-ERROR-resource-gre-modules-Brow.patch

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

r=me with nit fixed.

::: dom/apps/Webapps.jsm
@@ +1170,5 @@
>          " from a content process with no 'webapps-manage' privileges.");
>          return null;
>        }
>      }
> +    // And RegisterBEP requires "browser" priv's...

s/priv's/permission

Comment 31

[Mark Hammond [:markh] [:mhammond]]

Thanks!

https://hg.mozilla.org/integration/fx-team/rev/3f2617daac82

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3f2617daac82