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

RESOLVED FIXED in Firefox 32, Firefox OS v2.0

Status

Firefox OS
Runtime
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: zac, Assigned: fabrice)

Tracking

({qablocker, regression})

unspecified
2.0 S6 (18july)
x86_64
Linux
qablocker, regression
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:2.0+, firefox31 wontfix, firefox32 fixed, firefox33 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

(Whiteboard: [xfail])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

4 years ago
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
(Reporter)

Updated

4 years ago
Blocks: 1021732
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?
Component: Gaia → Runtime
Keywords: qawanted
(Reporter)

Comment 2

4 years ago
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
(Reporter)

Comment 3

4 years ago
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)
Taking this to take a look tomorrow.
Assignee: nobody → amac

Updated

4 years ago
Keywords: qawanted
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.
(Reporter)

Comment 6

4 years ago
Thanks :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 :)
Assignee: amac → nobody
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.
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,...)
Flags: needinfo?(jsmith)
Zac - Are we seeing the test failure seen in comment 0 on 2.0?
Flags: needinfo?(jsmith) → needinfo?(zcampbell)

Updated

4 years ago
Whiteboard: [xfail]
(Reporter)

Comment 11

4 years ago
(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
Flags: needinfo?(zcampbell)
QA blocker + regression, so we need this fixed.
blocking-b2g: --- → 2.0?
Keywords: qablocker, regression

Updated

4 years ago
blocking-b2g: 2.0? → 2.0+
status-b2g-v2.0: --- → affected
status-b2g-v2.1: --- → affected

Comment 13

4 years ago
Gregor, do you know who could own this bug? Thank you.
Flags: needinfo?(anygregor)

Comment 14

4 years ago
(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.
Flags: needinfo?(timdream)
(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!
Flags: needinfo?(timdream)

Comment 16

4 years ago
(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.
Gregor is on PTO, so I'm redirecting the needinfo to Candice.
Flags: needinfo?(anygregor) → needinfo?(cserran)
And I am redirecting to Fabrice who has worked on this (in Gregor's absence)
Flags: needinfo?(fabrice)
Flags: needinfo?(cserran)
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.
Assignee: nobody → kyle
Flags: needinfo?(fabrice)
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.
Depends on: 1015894
Flags: needinfo?(21)

Updated

4 years ago
Whiteboard: [xfail] → [xfail][systemsfe]
Target Milestone: --- → 2.0 S5 (4july)
ni?'ing fabrice too since he reviewed.
Flags: needinfo?(fabrice)
(Assignee)

Comment 22

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

Comment 23

4 years ago
hm, so we send the outer window ID from the child to the window watcher in the parent. No way this can work...
(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.
(Assignee)

Comment 25

4 years ago
Created attachment 8446965 [details] [diff] [review]
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.
Flags: needinfo?(bugs)
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.
Flags: needinfo?(bugs)
(Assignee)

Comment 27

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

Updated

4 years ago
Flags: needinfo?(bugs)
One option is to add the message listener on parent side to BrowserElementParent.js
Flags: needinfo?(bugs)
(Assignee)

Comment 29

4 years ago
Created attachment 8447707 [details] [diff] [review]
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.
Assignee: kyle → fabrice

Updated

4 years ago
Whiteboard: [xfail][systemsfe] → [xfail]
(Assignee)

Comment 30

4 years ago
Created attachment 8447819 [details] [diff] [review]
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!
Attachment #8446965 - Attachment is obsolete: true
Attachment #8447707 - Attachment is obsolete: true
(Assignee)

Comment 31

4 years ago
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!
Attachment #8447819 - Flags: feedback?(bugs)
RemotePermissionRequest doesn't have IID. So nsCOMPtr<RemotePermissionRequest> shouldn't be used, 
but nsRefPtr<RemotePermissionRequest>

Updated

4 years ago
Attachment #8447819 - Flags: feedback?(bugs)
(Assignee)

Comment 33

4 years ago
Created attachment 8451110 [details] [diff] [review]
cpp-prompt.patch

Olli, not sure who should review that. Feel free to redirect!
Attachment #8451110 - Flags: review?(bugs)
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)
Attachment #8451110 - Flags: review?(bugs) → review-
(Assignee)

Comment 35

4 years ago
Created attachment 8451899 [details] [diff] [review]
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...
Attachment #8447819 - Attachment is obsolete: true
Attachment #8451110 - Attachment is obsolete: true
Attachment #8451899 - Flags: review?(bugs)
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.
Attachment #8451899 - Flags: review?(bugs) → review+
(Assignee)

Comment 38

4 years ago
and backed out for build failures.

https://hg.mozilla.org/integration/b2g-inbound/rev/5944f23a2178
https://hg.mozilla.org/mozilla-central/rev/61ee3167a1d2
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: 2.0 S5 (4july) → 2.0 S6 (18july)
Needs rebasing for Aurora uplift.
status-b2g-v2.1: affected → fixed
status-firefox31: --- → wontfix
status-firefox32: --- → affected
status-firefox33: --- → fixed
Flags: needinfo?(fabrice)
Keywords: branch-patch-needed
(Assignee)

Comment 44

4 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/49ae43e813ce
status-b2g-v2.0: affected → fixed
status-firefox32: affected → fixed
Flags: needinfo?(fabrice)
Flags: needinfo?(21)
Keywords: branch-patch-needed
(Assignee)

Comment 46

4 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/db2b4035291d
status-b2g-v2.0: affected → fixed
status-firefox32: affected → fixed
(Reporter)

Comment 47

4 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

4 years ago
Blocks: 1037030
(Assignee)

Updated

4 years ago
No longer blocks: 1037030
Depends on: 1037030
(Assignee)

Updated

4 years ago
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
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.
You need to log in before you can comment on or make changes to this bug.