901214 - new Notification API: crashes on Firefox OS

Status: RESOLVED WORKSFORME

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

Description

[Julien Wajsberg [:julienw]]

STR:
* go to http://everlong.org/mozilla/notifications/ with the Browser
* press the second button "new Notification"

Expected:
* a notification is sent (the same one as the first button)

Actual:
* the page crashes

Note that I haven't tried the same thing in a app yet.

For this to happen you need to either have the patch for Bug 900960 or enable the webnotifications in Gaia's build/custom-prefs.js file.

Comment 1

[Julien Wajsberg [:julienw]]

Note that you need to have a gecko build from before the fix to bug 866232 (that regressed the Browser app) to be able to click the buttons

Comment 2

[Scoobidiver (away)]

Can you provide a crash ID (see https://wiki.mozilla.org/B2G/QA/Tips_And_Tricks#Getting_crashes_off_the_Device)?

Comment 3

[Julien Wajsberg [:julienw]]

Sorry, I don't have this directory on the device, even if I activate "always send reports" in Settings. And when I try to use run-gdb I have an error like "gdbserver: not found" and I can't get the backtrace.

I'll be happy to configure my device correctly if you have any information to do this.

Comment 4

[Reuben Morais [:reuben]]

I'm looking at this.

Comment 5

[Julien Wajsberg [:julienw]]

Thomas Zimmermann could not reproduce this on your device, please tell me if you can ;)

I had Thomas help me debugging this:
* I pushed a eng build to my keon and could then run gdb
* but I could not get any useful stack trace

And I'm stuck with adding some logs in all places ;)

Comment 6

[Julien Wajsberg [:julienw]]

There is a "meaningful" logcat error when it crashes:

08-05 17:08:21.177   753   753 E GeckoConsole: [JavaScript Error: "[Exception... "'Error: no message manager set' when calling method: [nsIObserver::observe]"  nsresult: "0x8057001c (NS_ERROR_XPC_JS_THREW_JS_OBJECT)"  location: "native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0"  data: no]"]

I'm currently instrumenting the code with logs everywhere.

Comment 7

[Julien Wajsberg [:julienw]]

Attachment: notification-debug.patch

so here are the logs that I added to debug so far, hope you'll find it useful.

Comment 8

[Gregor Wagner [:gwagner]]

GDB might be easier :)

Comment 9

[Gregor Wagner [:gwagner]]

I see
Timestamp: 8/5/13 6:09:26 PM
Error: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIContentPermissionRequest.element]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: file:///Users/Gregor/code/2src/2optbuild/dist/B2G.app/Contents/MacOS/components/ContentPermissionPrompt.js :: ContentPermissionPrompt.prototype.prompt :: line 128"  data: no]
Source File: file:///Users/Gregor/code/2src/2optbuild/dist/B2G.app/Contents/MacOS/components/ContentPermissionPrompt.js
Line: 128

Comment 10

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Gregor Wagner [:gwagner] from comment #8)
> GDB might be easier :)

We've been trying this all day, but GDB is very much unhelpful here :(

Comment 11

[Thomas Zimmermann [:tzimmermann] [:tdz]]

I found this in my logcat:

> I/Gecko   (  108): Security problem: Content process does not have `desktop-notification'.  It will be killed.
> I/Gecko   (  108): IPDL protocol error: Handler for ShowAlertNotification returned error code
> I/Gecko   (  108): 
> I/Gecko   (  108): ###!!! [Parent][AsyncChannel] Error: Processing error: message was deserialized, but the handler returned false (indicating failure)
> I/Gecko   (  108):

Comment 12

[Julien Wajsberg [:julienw]]

For the desktop-notification part you may need my patch from bug 900279 after all (I had it in my gaia).

Comment 13

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Julien Wajsberg [:julienw] from comment #12)
> For the desktop-notification part you may need my patch from bug 900279
> after all (I had it in my gaia).

The messages still show up even with the patch applied.

Comment 14

[Gregor Wagner [:gwagner]]

Attachment: Fix GetElement

Can you try with this patch?

Comment 15

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Gregor Wagner [:gwagner] from comment #14)
> Created attachment 785821 [details] [diff] [review]
> Fix GetElement
> 
> Can you try with this patch?

Didn't fix it for me.

Comment 16

[Thomas Zimmermann [:tzimmermann] [:tdz]]

And I also see the following messages in the logcat. Pid 494 is the Browser app.

> I/Gecko   (  109): [Parent 109] WARNING: waitpid failed pid:494 errno:10: file /home/mozilla/Projects/mozilla/src/mozilla-central/ipc/chromium/src/base/process_util_posix.cc, line 260
> I/Gecko   (  109): [Parent 109] WARNING: Failed to deliver SIGKILL to 494!(3).: file /home/mozilla/Projects/mozilla/src/mozilla-central/ipc/chromium/src/chrome/common/process_watcher_posix_sigchld.cc, line 118

Comment 17

[Gregor Wagner [:gwagner]]

Attachment: fix GetElement + ContentPermissionPrompt.js

Some more fixes

Comment 18

[Gregor Wagner [:gwagner]]

createNotification works now for me on b2g-desktop.
"new Notification" doesn't handle prompting correctly. It works only after using createNotification.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Uh.  aElement should never be null there.  Why is it null?

Comment 20

[Caio Lima(:caiolima)]

May it could be helpful.

I've tried to construct the Notification with "var notification=Notification.constructor(...)" and It worked for me. The Object was created, but the 'desktop-notification' message wasn't sent to system app.

I think that it's is strange, because I've tested in Nightly app and the "new Notification" works.

I'm testing it in the sms application.

Comment 21

[Julien Wajsberg [:julienw]]

> createNotification works now for me on b2g-desktop.

The old spec always worked for me on the device :)

It's the new spec which crashes for me.

Gregor have you applied all the not-landed-yet patches for the new Notification API before trying ?

Comment 22

[Caio Lima(:caiolima)]

>createNotification works now for me on b2g-desktop.
For me too. But It's the old implementation.

Julien, have you already tested the "new Notification(..)" with a gaia app, like sms?

I would like to know if you will get the same error than mine. "ErrorType: Notification is not a constructor"

Comment 23

[Julien Wajsberg [:julienw]]

Bug 901856 brings the notification api to the UI Tests app. The notification is correctly sent, but there are other bugs.

So maybe this bug is related to the fact that Browser tabs are remote ?

Comment 24

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Gregor Wagner [:gwagner] from comment #17)
> Created attachment 785849 [details] [diff] [review]
> fix GetElement + ContentPermissionPrompt.js
> 
> Some more fixes

Still no change here.

Comment 25

[Gregor Wagner [:gwagner]]

(In reply to Boris Zbarsky (:bz) from comment #19)
> Uh.  aElement should never be null there.  Why is it null?

Hm good question. I copied this behavior from geolocation: http://mxr.mozilla.org/mozilla-central/source/dom/src/geolocation/nsGeolocation.cpp#401

There is some explanation here:
http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIContentPermissionPrompt.idl#43

The current use-case is that the child sends a prompting request to the parent.
We handle the case where we don't have an element but currently we return an error:
http://mxr.mozilla.org/mozilla-central/source/b2g/components/ContentPermissionPrompt.js#131

Comment 26

[Gregor Wagner [:gwagner]]

(In reply to Julien Wajsberg [:julienw] from comment #21)
> > createNotification works now for me on b2g-desktop.
> 
> The old spec always worked for me on the device :)
> 
> It's the new spec which crashes for me.
> 
> Gregor have you applied all the not-landed-yet patches for the new
> Notification API before trying ?

I enabled them and applied https://bug782211.bugzilla.mozilla.org/attachment.cgi?id=783252
Anything else missing?

Comment 27

[Caio Lima(:caiolima)]

Julien, I've applied your patch to debug, but I think that I have no success. Where the debug messages are showed? I'm testing on emulator, because I don't have a phone. The messages aren't being sent to logcat and console.

Comment 28

[Julien Wajsberg [:julienw]]

Even on emulator you may need to enable the console in the Settings app ? Sorry I don't know the emulator...

Comment 29

[Caio Lima(:caiolima)]

hum...I think so.  I'll test it now.

Comment 30

[Boris Zbarsky [:bzbarsky]]

(In reply to Gregor Wagner [:gwagner] from comment #25)
> I copied this behavior from geolocation:

Well, that code is just as bogus.  It's trying to defend-in-depth against things that are totally not allowed.

> There is some explanation here:
> http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/base/
> nsIContentPermissionPrompt.idl#43

That's talking about the return value.  You're checking the pointer to the memory location where the return value should be written.

Comment 31

[Gregor Wagner [:gwagner]]

The new code doesn't support any prompting. We have to fix this.
http://mxr.mozilla.org/mozilla-central/source/dom/src/notification/Notification.cpp#400

Comment 32

[Reuben Morais [:reuben]]

(In reply to Gregor Wagner [:gwagner] from comment #31)
> The new code doesn't support any prompting. We have to fix this.
> http://mxr.mozilla.org/mozilla-central/source/dom/src/notification/
> Notification.cpp#400

Prompting is done in Notification.requestPermission: https://mxr.mozilla.org/mozilla-central/source/dom/src/notification/Notification.cpp#105
The constructor should just do nothing if there's no permission.

Comment 33

[Caio Lima(:caiolima)]

Guys, does anybody testing it on Desktop B2G Simulator or plug-in simulator?

Comment 34

[Reuben Morais [:reuben]]

Attachment: WIP

WIP, fixes the crash. I think bent and sicking have a plan to fix this differently, though.

Comment 35

[Reuben Morais [:reuben]]

Attachment: WIP

Missing qref.

Comment 36

[Caio Lima(:caiolima)]

Reuben, there is an error in attachment 787264 [details] [diff] [review]. On dom/src/notification/Notification.cpp:323 line, the new method "Notification::GetPrincipal(nsIPrincipal** aPrincipal)" has a parameter that wasn't declared as the method on "dom/src/notification/Notification.h:60". It is declared this way "nsIPrincipal* GetPrincipal();"

It generated a compilation error. So, I've removed the "nsIPrincipal** aPrincipal" from "Notification.cpp" and It compiled well.

Comment 37

[Reuben Morais [:reuben]]

I thought I had removed that before attaching it. Should work fine other than that. Try run looks green: https://tbpl.mozilla.org/?tree=Try&rev=cd764bc7b55b

Comment 38

[Caio Lima(:caiolima)]

Sorry for this newbie ask, but what is the meaning of this link?

https://tbpl.mozilla.org/?tree=Try&rev=cd764bc7b55b

Comment 39

[Boris Zbarsky [:bzbarsky]]

Caio, see https://wiki.mozilla.org/ReleaseEngineering/TryServer#Try_Server

The link is to a particular set test runs, presumbly with the patch for this bug.

Comment 40

[Candice Serran (:cserran)]

Candice Serran changed story state to started in Pivotal Tracker

Comment 41

[[:fabrice] Fabrice Desré]

Comment on attachment 787264 [details] [diff] [review]
WIP

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

::: dom/ipc/ContentParent.cpp
@@ +2423,5 @@
> +      found = true;
> +      break;
> +    }
> +  }
> +

You should use the AppsService there, and just check that getManifestURLByLocalId(principalAppId) != null to find if we have an app with this id.

Comment 42

[Caio Lima(:caiolima)]

I guess I've found a kind of problem. I was trying to build my Gecko setting the GECKO_PATH to mozilla-central that I had here and the "new Notification(...)" was reporting to logcat that "Notification is not a constructor". 
Today, Reuben said to me to try build my b2g with "BRANCH=master ./config.sh" and the "new Notification(...)" worked well. There is an explanation for this behavior?

Comment 43

[Julien Wajsberg [:julienw]]

I use the GECKO_PATH configuration too, but in this situation you need to update the repository yourself instead of relying on "./repo sync" in B2G's directory. Maybe that was the problem ?

Comment 45

[Reuben Morais [:reuben]]

Is this still reproducing on anyone's device? After rebasing my patches notifications are not leaving the child at any point, completely circumventing the permissions system. I'm puzzled.

Comment 46

[Jason Smith [:jsmith]]

Attachment: Test Case

Comment 47

[Jason Smith [:jsmith]]

(In reply to Reuben Morais [:reuben] from comment #45)
> Is this still reproducing on anyone's device? After rebasing my patches
> notifications are not leaving the child at any point, completely
> circumventing the permissions system. I'm puzzled.

Nope. Works for me on a 9/12/2013 build.

Comment 48

[Candice Serran (:cserran)]

Jason Smith changed story state to finished in Pivotal Tracker

Comment 49

[Candice Serran (:cserran)]

Jason Smith changed story state to accepted in Pivotal Tracker

Comment 50

[Candice Serran (:cserran)]

Jason Smith changed story state to accepted in Pivotal Tracker