1024513 - JavaScript Error: "NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: [JavaScript Error: "targetElement is null"

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Runtime, defect)

Description

[Zac C (:zac)]

STR:

For the app "UI tests - Privileged App", set the permission 'contacts-create' to 'prompt'

1. Load "UI tests - Privileged App"
2. tap "Api"
3. tap "Contacts"
4. tap "Insert fake contacts"

Nothing happens, no permissins prompt is shown nor are contacts added.

This exception is in logcat:
E/GeckoConsole( 7338): [JavaScript Error: "NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: [JavaScript Error: "targetElement is null" {file: "jar:file:///system/b2g/omni.ja!/components/ContentPermissionPrompt.js" line: 434}]'[JavaScript Error: "targetElement is null" {file: "jar:file:///system/b2g/omni.ja!/components/ContentPermissionPrompt.js" line: 434}]' when calling method: [nsIContentPermissionPrompt::prompt]" {file: "jar:file:///system/b2g/omni.ja!/components/PermissionPromptService.js" line: 84}]

Device:
Gaia      41db6954a67efc55016744bc8f6591ae9e07a285
Gecko     https://hg.mozilla.org/mozilla-central/rev/9e8e3e903484
BuildID   20140612040203
Version   33.0a1
ro.build.version.incremental=eng.cltbld.20140611.105510
ro.build.date=Wed Jun 11 10:55:20 EDT 2014

The build includes the patch from:
https://bugzilla.mozilla.org/show_bug.cgi?id=1014410

Comment 1

[Jason Smith [:jsmith]]

Can we include end user STR here to reproduce this using an eng build, the UI Tests - Privileged App, and the settings app permissions UI for settings contacts to prompt?

Comment 2

[Zac C (:zac)]

I'm not sure how to set the permission to prompt - it is no longer an option from within the Settings app.

I'll see if it can be done using the App Manager

Comment 3

[Zac C (:zac)]

To set the permission:
1. Load App Manager with 'debug certified apps' pref set.
2. Debug System App
3. In System app's console, paste:
navigator.mozPermissionSettings.set('contacts-create', 'prompt', 'app://uitest-privileged.gaiamobile.org/manifest.webapp', 'app://uitest-privileged.gaiamobile.org', false)

Comment 4

[Antonio Manuel Amaya Calvo (:amac)]

Taking this to take a look tomorrow.

Comment 5

[Antonio Manuel Amaya Calvo (:amac)]

Ok, there seem to be two bugs here. And both are unrelated with bug 1014410 :). In fact, for engineering builds, bug 1014410 is a NOP (doesn't do anything since for engineering builds all applications are considered to be *not* preinstalled so they're installed as PROMPT).

One bug is with bug 1024940 now. And this one seems to be a bug on PermissionPrompt.

Comment 6

[Zac C (:zac)]

Thanks :amac!

Comment 7

[Antonio Manuel Amaya Calvo (:amac)]

Ok, I tracked this up to:

https://mxr.mozilla.org/mozilla-central/source/dom/permission/PermissionPromptHelper.jsm#103

msg.windowID has a value, but request.window is null.

I won't have time to check more on this today (and I don't know the window manager code), so I'm releasing this. I'll retake this weekend or on Monday if none comes out and takes it :)

Comment 8

[Antonio Manuel Amaya Calvo (:amac)]

Oh one last thing for today :P

According to https://mxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIContentPermissionPrompt.idl#53 it's quite normal that window is null since we're running OOP. But then element should be not null. And nobody is initializing element though.

Comment 9

[Antonio Manuel Amaya Calvo (:amac)]

I don't know if the tag I want is qawanted, but someone should test this also on 2.0. If it's broken there also, this should be a blocker (cause privileged apps won't be able to use contacts, device storage,...)

Comment 10

[Jason Smith [:jsmith]]

Zac - Are we seeing the test failure seen in comment 0 on 2.0?

Comment 11

[Zac C (:zac)]

(In reply to Jason Smith [:jsmith] from comment #10)
> Zac - Are we seeing the test failure seen in comment 0 on 2.0?

Yes, it occurs on v2.0 too!

Checked with:
Device    Flame
Gaia      0f254c92bc44d614ae56a855f18a895a7e4703ad
Gecko     https://hg.mozilla.org/releases/mozilla-aurora/rev/7f309a3a4d3d
BuildID   20140617000201
Version   32.0a2
ro.build.version.incremental=108
ro.build.date=Tue Jun 10 19:40:40 CST 2014

Comment 12

[Jason Smith [:jsmith]]

QA blocker + regression, so we need this fixed.

Comment 13

[Kai-Chih Hu [:khu]]

Gregor, do you know who could own this bug? Thank you.

Comment 14

[Kai-Chih Hu [:khu]]

(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #5)
> Ok, there seem to be two bugs here. And both are unrelated with bug 1014410
> :). In fact, for engineering builds, bug 1014410 is a NOP (doesn't do
> anything since for engineering builds all applications are considered to be
> *not* preinstalled so they're installed as PROMPT).
> 
> One bug is with bug 1024940 now. And this one seems to be a bug on
> PermissionPrompt.

Tim, I am not sure if this is something George Duan can help? Can we have your comment / suggestion on this bug? Thank you.

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Kevin Hu [:khu] from comment #14)
> Tim, I am not sure if this is something George Duan can help? Can we have
> your comment / suggestion on this bug? Thank you.

These two bugs are not related. I would be very surprised if something in the platform can be broken by a simple Settings app UI breakage!

Comment 16

[Kai-Chih Hu [:khu]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #15)
> I would be very surprised if something in
> the platform can be broken by a simple Settings app UI breakage!

I agreed with you. Of course, it's worthy to check with you.

Comment 17

[Jason Smith [:jsmith]]

Gregor is on PTO, so I'm redirecting the needinfo to Candice.

Comment 18

[Candice Serran (:cserran)]

And I am redirecting to Fabrice who has worked on this (in Gregor's absence)

Comment 19

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Ok, it looks like the contacts API is the only thing using the PermissionsPromptHelper, which assembles a nsContentPermissionRequest on the parent side after receiving an IPC message. PermissionsPromptHelper doesn't actually pack the element, just the window, which is null because we're OOP. Unfortunately trying to pack the message.target as the element also throws an error since it's the mm, not the originating element.

It looks like nsContentPermissionRequest has an IPDL interface now that does all of this, but I'm not sure if we can use that? Also not sure what changed to break this exactly, should probably bisect.

Comment 20

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Agh. Looking at the right branch helps when trying to blame things. 

This bug is due to bug 1015894, which is where the request.element call came in to ContentPermissionPrompt.sendToBrowserWindow. Note that we even check if request.element is null and if it is THEN call delegatePrompt (which calls sendToBrowserWindow) at line 299, so I'm not sure how this ever worked in an OOP context?

ni'ing Vivien for more info.

Comment 21

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

ni?'ing fabrice too since he reviewed.

Comment 22

[[:fabrice] Fabrice Desré]

We fail because http://mxr.mozilla.org/mozilla-central/source/dom/permission/PermissionPromptHelper.jsm#103 returns null. Investigating whether the nsIWindowWatcher component is actually usable from a content process.

Comment 23

[[:fabrice] Fabrice Desré]

hm, so we send the outer window ID from the child to the window watcher in the parent. No way this can work...

Comment 24

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(In reply to Fabrice Desré [:fabrice] from comment #23)
> hm, so we send the outer window ID from the child to the window watcher in
> the parent. No way this can work...

I don't think we're expected to be able to resolve off the window ID in the OOP case. Check out the comments at http://mxr.mozilla.org/mozilla-central/source/b2g/components/ContentPermissionPrompt.js#424 - it's expecting element to be defined in the OOP case, which its not here.

Comment 25

[[:fabrice] Fabrice Desré]

Attachment: wip patch

This patch uses the frame manager from the content process window to let the parent get a handle to the right window too.

For some reason that doesn't work though: the global message manager in the parent never receives any message. Olli, anything that looks wrong in our setup there? PermissionPromptHelper runs in the parent and is initialized, while ContactManager is a content side xpcom.

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

What is the window object. Something under mozbrowser and the parent side is also some
html doc? such mozbrowsers aren't in the message manager tree where global is the root.
nsFrameLoader gives a message manager (parent side) a parent, if it finds a mm from its
ChromeWindow.

Comment 27

[[:fabrice] Fabrice Desré]

(In reply to Olli Pettay [:smaug] from comment #26)
> What is the window object. Something under mozbrowser and the parent side is
> also some
> html doc? such mozbrowsers aren't in the message manager tree where global
> is the root.

Yes, the window is a <iframe mozbrowser mozapp remote>, the parent is gaia's system app.

> nsFrameLoader gives a message manager (parent side) a parent, if it finds a
> mm from its
> ChromeWindow.

hm... so what can we do here?

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

One option is to add the message listener on parent side to BrowserElementParent.js

Comment 29

[[:fabrice] Fabrice Desré]

Attachment: patch v2

Try run at https://tbpl.mozilla.org/?tree=Try&rev=2ba11c010e8a (hm, debug builds are burning...)

I didn't follow the BEP.js way because we also need to support that at least in the android runtime that doesn't use mozbrowser frames. So this patch exposes a c++ version (inspired by MediaPermissionGonk.cpp) on nsIDOMWindowUtils and takes care of the remoting.

Comment 30

[[:fabrice] Fabrice Desré]

Attachment: patch v3

Some clean up and adding support for choices in the Allow() callback.

That is still failing in debug builds with this error:

In file included from ../../dist/include/nsIWeakReferenceUtils.h:9:0,
                 from ../../dist/include/nsWeakReference.h:14,
                 from /home/fabrice/dev/birch/dom/base/nsDOMWindowUtils.h:9,
                 from /home/fabrice/dev/birch/dom/base/nsDOMWindowUtils.cpp:6:
../../dist/include/nsCOMPtr.h: In instantiation of ‘nsCOMPtr<T>::nsCOMPtr(nsQueryInterface) [with T = RemotePermissionRequest]’:
../../dist/include/nsCOMPtr.h:521:58:   required from ‘void nsCOMPtr<T>::Assert_NoQueryNeeded() [with T = RemotePermissionRequest]’
../../dist/include/nsCOMPtr.h:559:26:   required from ‘nsCOMPtr<T>::nsCOMPtr(T*) [with T = RemotePermissionRequest]’
/home/fabrice/dev/birch/dom/base/nsDOMWindowUtils.cpp:3694:49:   required from here
../../dist/include/nsCOMPtr.h:607:65: error: incomplete type ‘nsIContentPermissionRequest::COMTypeInfo<RemotePermissionRequest, void>’ used in nested name specifier
     assign_from_qi(aQI, NS_GET_TEMPLATE_IID(T));
                                                                 ^

help!

Comment 31

[[:fabrice] Fabrice Desré]

Comment on attachment 8447819 [details] [diff] [review]
patch v3

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

Olli, I went with doing the remoting in c++ because that also needs to work for frames that are not <mozbrowser> (eg on Android). If you have any idea about the compilation issue in debug builds that would be great!

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

RemotePermissionRequest doesn't have IID. So nsCOMPtr<RemotePermissionRequest> shouldn't be used, 
but nsRefPtr<RemotePermissionRequest>

Comment 33

[[:fabrice] Fabrice Desré]

Attachment: cpp-prompt.patch

Olli, not sure who should review that. Feel free to redirect!

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8451110 [details] [diff] [review]
cpp-prompt.patch

>+already_AddRefed<nsPIDOMWindow>
>+RemotePermissionRequest::GetOwner()
>+{
>+  return mWindow.forget();
>+}
I don't see any need for mWindow nor GetOwner()


>+
>+// PCOMContentPermissionRequestChild
>+bool
>+RemotePermissionRequest::Recv__delete__(const bool& allow,
>+                                        const InfallibleTArray<PermissionChoice>& choices)
Shouldn't this be just const nsTArray<PermissionChoice>&

And param names should be in form aFoo

>+{
>+  if (allow) {
>+    // Convert choices to a JS val if any.
>+    // {"type1": "choice1", "type2": "choiceA"}
unfortunate API :/
Would be so much nicer if we could define this kind of dictionaries using webidl
and the array as sequence<somedictionary>. But no need to change this stuff in this bug.


>+    AutoSafeJSContext cx;
AutoJSAPI is the new magic.

>+    JS::Rooted<JSObject*> obj(cx);
>+    obj = JS_NewObject(cx, nullptr, JS::NullPtr(), JS::NullPtr());
>+    JSAutoCompartment ac(cx, obj);
>+    for (uint32_t i = 0; i < choices.Length(); ++i) {
>+      const nsString &choice = choices[i].choice();
>+      const nsCString &type = choices[i].type();
& goes with the type, not with variable name.

>+      JS::Rooted<JSString*> jChoice(cx, JS_NewUCStringCopyN(cx, choice.get(), choice.Length()));
>+      JS::Rooted<JS::Value> vChoice(cx, STRING_TO_JSVAL(jChoice));
>+      JS_SetProperty(cx, obj, type.get(), vChoice);
I think you could just do JS_SetProperty(cx, obj, type.get(), JS::StringValue(jChoice));



>+    JS::Rooted<JS::Value> v(cx);
>+    (void) Allow(v);
I don't understand this.
You just pass v to Allow. Don't you want Allow(JS::ObjectValue(obj))


>+class RemotePermissionRequest : public nsIContentPermissionRequest
>+                              , public PCOMContentPermissionRequestChild
>+{
>+public:
>+  NS_DECL_ISUPPORTS
>+  NS_DECL_NSICONTENTPERMISSIONREQUEST
>+
>+  RemotePermissionRequest(nsIContentPermissionRequest* aRequest,
>+                          nsPIDOMWindow* aWindow);
>+  virtual ~RemotePermissionRequest() {}
>+
>+  // It will be called when prompt dismissed.
>+  virtual bool Recv__delete__(const bool &allow,
>+                              const InfallibleTArray<mozilla::dom::PermissionChoice>& choices) MOZ_OVERRIDE;
>+  virtual void IPDLRelease() MOZ_OVERRIDE { Release(); }
uhuh, IPDLRelease stuff is so black magic. /me kicks dougt.
Could you file a followup to remove IPDLRelease stuff.
Using IPDLRelease in this patch is still fine.

>+nsDOMWindowUtils::GetPermission(nsIContentPermissionRequest* aRequest)
I'd prefer calling this AskPermission.
Get* hints like some kind of getter method.


>+{
>+  nsCOMPtr<nsPIDOMWindow> window = do_QueryReferent(mWindow);
>+  nsRefPtr<RemotePermissionRequest> req =
>+    new RemotePermissionRequest(aRequest, window);
>+
>+    // for content process
>+  if (XRE_GetProcessType() == GeckoProcessType_Content) {
>+    MOZ_ASSERT(NS_IsMainThread()); // IPC can only be execute on main thread.
>+
>+    nsresult rv;
>+
>+    nsCOMPtr<nsPIDOMWindow> window(req->GetOwner());
>+    NS_ENSURE_TRUE(window, NS_ERROR_FAILURE);
So this is the only place where you use GetOwner, and it just returns mWindow of
DOMWindowUtils


>+  // for chrome process
>+  nsCOMPtr<nsIContentPermissionPrompt> prompt =
>+      do_GetService(NS_CONTENT_PERMISSION_PROMPT_CONTRACTID);
use 2 spaces for indentation


I'd like to see another patch. (quick review promised)

Comment 35

[[:fabrice] Fabrice Desré]

Attachment: cpp-prompt.patch

Olli, I addressed all your comments except :

> I think you could just do JS_SetProperty(cx, obj, type.get(), JS::StringValue(jChoice));

There's maybe another jsapi trick we could use...

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8451899 [details] [diff] [review]
cpp-prompt.patch

>+bool
>+RemotePermissionRequest::Recv__delete__(const bool& aAllow,
>+                                        const nsTArray<PermissionChoice>& aChoices)
>+{
>+  if (aAllow) {
>+    // Convert choices to a JS val if any.
>+    // {"type1": "choice1", "type2": "choiceA"}
>+    AutoJSAPI jsapi;
>+      if (NS_WARN_IF(!jsapi.Init(mWindow))) {
2 extra spaces

>+      return true; // This is not an IPC error.
>+    }
>+    JSContext* cx = jsapi.cx();
>+    JS::Rooted<JSObject*> obj(cx);
>+    obj = JS_NewObject(cx, nullptr, JS::NullPtr(), JS::NullPtr());
>+    JSAutoCompartment ac(cx, obj);
jsapi.Init should take care of compartment entering 


>+    for (uint32_t i = 0; i < aChoices.Length(); ++i) {
>+      const nsString& choice = aChoices[i].choice();
>+      const nsCString& type = aChoices[i].type();
>+      JS::Rooted<JSString*> jChoice(cx, JS_NewUCStringCopyN(cx, choice.get(), choice.Length()));
>+      JS::Rooted<JS::Value> vChoice(cx, STRING_TO_JSVAL(jChoice));
Hmm, can you at least here use the StringValue? Use of those old macros feel wrong

>+      JS_SetProperty(cx, obj, type.get(), vChoice);
Check the return value and if false, return false.




>+NS_IMETHODIMP
>+nsDOMWindowUtils::AskPermission(nsIContentPermissionRequest* aRequest)
>+{
>+  nsCOMPtr<nsPIDOMWindow> window = do_QueryReferent(mWindow);
>+  nsRefPtr<RemotePermissionRequest> req =
>+    new RemotePermissionRequest(aRequest, window);
This is annoying, but I think we need to store inner window in 
RemotePermissionRequest, and when
getting the response check that the window is still the current inner window, 
(IsCurrentInnerWindow()) and if it is not, return early.

Comment 37

[[:fabrice] Fabrice Desré]

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

Comment 38

[[:fabrice] Fabrice Desré]

and backed out for build failures.

https://hg.mozilla.org/integration/b2g-inbound/rev/5944f23a2178

Comment 39

[[:fabrice] Fabrice Desré]

Relanded at https://hg.mozilla.org/integration/b2g-inbound/rev/a2f2a28e0856 after a green try (https://tbpl.mozilla.org/?tree=Try&rev=ff06d3f5458a)

Comment 40

[Ed Morley [:emorley]]

Backed out for build failures:
https://tbpl.mozilla.org/php/getParsedLog.php?id=43344848&tree=B2g-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=43345026&tree=B2g-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=43344878&tree=B2g-Inbound

remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/4254641ca600

Comment 41

[[:fabrice] Fabrice Desré]

3rd time's a charm:
https://hg.mozilla.org/integration/b2g-inbound/rev/61ee3167a1d2

Comment 42

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

https://hg.mozilla.org/mozilla-central/rev/61ee3167a1d2

Comment 43

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for Aurora uplift.

Comment 44

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/releases/mozilla-aurora/rev/49ae43e813ce

Comment 45

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
https://hg.mozilla.org/releases/mozilla-aurora/rev/94047c419c3f

https://tbpl.mozilla.org/php/getParsedLog.php?id=43449859&tree=Mozilla-Aurora

Comment 46

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/releases/mozilla-aurora/rev/db2b4035291d

Comment 47

[Zac C (:zac)]

We attempted enabling our tests on Travis today but struck problems.

I debugged it and found this error message in the console:
Error: perm.options is null
Source File: file:///home/zac/Mozilla/gaia/b2g/components/ContentPermissionPrompt.js
Line: 28

This is on TBPL mozilla-central desktopb2g builds.

STR are:
1. Download build from http://ftp.mozilla.org/pub/mozilla.org/b2g/tinderbox-builds/mozilla-central-linux64_gecko/1405001967/
2. make a gaia profile from master branch: "make"
3. Load ./b2g-bin -profile /path/to/gaia/profile/
4. Complete FTU
5. Open "UI tests - privileged" app
6. Tap Contacts
7. Tap Insert fake contacts
8. Note error message.

Comment 48

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

This patch breaks the Contacts permission prompts in Fennec (bug 1037128) very badly. We need to fix quickly or back this out of m-c and m-a.