Closed Bug 1024513 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox OS Graveyard :: Runtime, defect)

x86_64
Linux
defect
Not set

Tracking

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

RESOLVED FIXED
2.0 S6 (18july)
blocking-b2g 2.0+
Tracking Status
firefox31 --- wontfix
firefox32 --- fixed
firefox33 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: zcampbell, Assigned: fabrice)

References

Details

(Keywords: qablocker, regression, Whiteboard: [xfail])

Attachments

(1 file, 4 obsolete files)

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
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
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
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
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.
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)
Whiteboard: [xfail]
(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?
blocking-b2g: 2.0? → 2.0+
Gregor, do you know who could own this bug? Thank you.
Flags: needinfo?(anygregor)
(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)
(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)
Whiteboard: [xfail] → [xfail][systemsfe]
Target Milestone: --- → 2.0 S5 (4july)
ni?'ing fabrice too since he reviewed.
Flags: needinfo?(fabrice)
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)
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.
Attached patch wip patch (obsolete) — Splinter Review
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)
(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?
Flags: needinfo?(bugs)
One option is to add the message listener on parent side to BrowserElementParent.js
Flags: needinfo?(bugs)
Attached patch patch v2 (obsolete) — Splinter Review
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
Whiteboard: [xfail][systemsfe] → [xfail]
Attached patch patch v3 (obsolete) — Splinter Review
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
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>
Attachment #8447819 - Flags: feedback?(bugs)
Attached patch cpp-prompt.patch (obsolete) — Splinter Review
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-
Attached patch cpp-prompt.patchSplinter Review
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+
https://hg.mozilla.org/mozilla-central/rev/61ee3167a1d2
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: 2.0 S5 (4july) → 2.0 S6 (18july)
Needs rebasing for Aurora uplift.
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 → ---
Blocks: 1037030
No longer blocks: 1037030
Depends on: 1037030
Status: REOPENED → RESOLVED
Closed: 6 years ago6 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.