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

RESOLVED FIXED in mozilla35

Status

()

RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: RyanVM, Assigned: fabrice)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla35
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

5 years ago
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
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...
Flags: needinfo?(fabrice)
(Assignee)

Comment 2

5 years ago
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.
Flags: needinfo?(fabrice)
(Reporter)

Comment 3

5 years ago
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).
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...
Flags: needinfo?(khuey)
I hesitate to say it's impossible, but it seems unlikely.  I would not expect problems caused by compartment sharing to be intermittent either.
Flags: needinfo?(khuey)
(Reporter)

Comment 6

5 years ago
This isn't intermittent. It happens on every run. It just currently isn't a fatal exception.
(Reporter)

Comment 7

5 years ago
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! :)
Flags: needinfo?(justin.lebar+bug)

Updated

5 years ago
No longer blocks: 892958

Updated

5 years ago
Blocks: 910614
I don't think I'm going to get to this before I leave on Friday.  Sorry.  :(
Flags: needinfo?(justin.lebar+bug)
(Reporter)

Comment 9

5 years ago
Back on to khuey's ever-growing heap then, I guess :-\
Flags: needinfo?(khuey)
(Assignee)

Comment 10

5 years ago
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
(Assignee)

Comment 11

5 years ago
Created attachment 797894 [details] [diff] [review]
patch
Attachment #797894 - Flags: review?(justin.lebar+bug)
Comment on attachment 797894 [details] [diff] [review]
patch

bizarre, but if it works, ship it!
Attachment #797894 - Flags: review?(justin.lebar+bug) → review+
Flags: needinfo?(khuey)
(Assignee)

Comment 13

5 years ago
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 :(

Updated

5 years ago
Blocks: 920191

Updated

5 years ago
No longer blocks: 910614
(Reporter)

Comment 14

5 years ago
Sorry to bug you Fabrice, but any news to report? :)
(Assignee)

Comment 17

5 years ago
Created attachment 8339505 [details] [diff] [review]
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
Assignee: nobody → fabrice
Attachment #797894 - Attachment is obsolete: true
Attachment #8339505 - Flags: review?(khuey)
(Assignee)

Comment 18

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

Tests don't look happy.
Attachment #8339505 - Flags: review?(khuey)
(Assignee)

Comment 19

5 years ago
Created attachment 8395059 [details] [diff] [review]
rebased patch

Moving that patch in the right bug.

Try run at https://tbpl.mozilla.org/?tree=Try&rev=adbc5e34ff11
Attachment #8339505 - Attachment is obsolete: true
Attachment #8395059 - Flags: review?(bent.mozilla)

Updated

5 years ago
Blocks: 961317
(Assignee)

Comment 20

5 years ago
Comment on attachment 8395059 [details] [diff] [review]
rebased patch

try doesn't seem happy...

Updated

5 years ago
Blocks: 974270
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 :)
Attachment #8395059 - Flags: review?(bent.mozilla)
Created attachment 8427342 [details] [diff] [review]
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
Attachment #8395059 - Attachment is obsolete: true
(Assignee)

Comment 23

5 years ago
/me facepalms

Comment 24

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

Blame JS! :)
(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)
(Assignee)

Updated

5 years ago
Attachment #8427342 - Flags: review?(bent.mozilla)
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.
Attachment #8427342 - Flags: review?(bent.mozilla) → review+
Created attachment 8493442 [details] [diff] [review]
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?
Attachment #8427342 - Attachment is obsolete: true
Attachment #8493442 - Flags: review?(fabrice)
(Assignee)

Comment 28

4 years ago
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
Attachment #8493442 - Flags: review?(fabrice) → review-

Updated

4 years ago
Blocks: 1067229
Created attachment 8494256 [details] [diff] [review]
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
Attachment #8493442 - Attachment is obsolete: true
Attachment #8494256 - Flags: review?(fabrice)
(Assignee)

Comment 30

4 years ago
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
Attachment #8494256 - Flags: review?(fabrice) → review+
(Reporter)

Comment 32

4 years ago
https://hg.mozilla.org/mozilla-central/rev/3f2617daac82
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.