bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

[system-message-api] SystemMessageManager makes JS Error: "this._dispatchers is null"

RESOLVED FIXED in Firefox 32, Firefox OS v2.0

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Gene Lian (I already quit Mozilla), Assigned: hchang)

Tracking

32 Branch
mozilla33
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

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

Details

(Whiteboard: [planned-sprint][in-sprint=v2.0-S5][p=3][ft:ril])

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

4 years ago
Symptom:

E/GeckoConsole(  709): [JavaScript Error: "this._dispatchers is null" {file: "jar:file:///system/b2g/omni.ja!/components/SystemMessageManager.js" line: 225}]

Please refer to bug 1030550, comment #c8 to reproduce this:

=== Steps to reproduce ===
1. Make a phone call. (Do not answer it.)
2. Hang up.
3. Make a phone call again.
4. Hang up.

This bug *might be* the root cause of bug 1030550 but we are not 100% sure yet, since we can produce this without using the BT.
(Reporter)

Comment 1

4 years ago
> Please refer to bug 1030550, comment #c8 to reproduce this:
                  ^^^^^^^^^^^^^^^^^^^^^^^^ I mean bug 1030550, comment #8
(Reporter)

Updated

4 years ago
Summary: [system-message-api] SystemMessageManager will make JavaScript Error → [system-message-api] SystemMessageManager makes JS Error: "this._dispatchers is null"
nominate because it blocks bug 1030550
blocking-b2g: --- → 2.0?
Not sure if Henry has bandwidth to take this.
Flags: needinfo?(hchang)
(Assignee)

Comment 4

4 years ago
(In reply to Wesley Huang [:wesley_huang] from comment #3)
> Not sure if Henry has bandwidth to take this.

I am sure Henry has~
Flags: needinfo?(hchang)

Comment 5

4 years ago
assign to Henry then, thanks!
Assignee: nobody → hchang
(Assignee)

Comment 6

4 years ago
Created attachment 8451563 [details]
diagnostic.txt

According to the log, SystemMessageManager with innerWindoID 8 unexpectedly received message from SystemMessageInternal while SystemMessageManager::uninit() has been called. (That's why |this._dispatchers| is set to null)

Checking DOMRequestIpcHelper::destroyDOMRequestHelper(), it did call cpmm.removeMessageListener to remove registered messages but it still received messages.
I am guessing the message dispatch and handling is not synchronous so we are not able to remove those dispatched-but-not-handled messages. If it is the root cause, the simplest solution is to check if |uninit| has been called by checking |this._dispatchers| or |this._destroyed|.
(Assignee)

Comment 7

4 years ago
Created attachment 8451570 [details]
diagnostic.txt
Attachment #8451563 - Attachment is obsolete: true
(Assignee)

Comment 8

4 years ago
Created attachment 8451580 [details] [diff] [review]
Bug1035074.patch

Attached sample solution. Requires someone to verify the original bug. Jamin, could you please do me this favor? Thanks!
Flags: needinfo?(jaliu)

Updated

4 years ago
Component: General → DOM: Device Interfaces
Product: Firefox OS → Core
Version: unspecified → 32 Branch
(Reporter)

Comment 9

4 years ago
Comment on attachment 8451580 [details] [diff] [review]
Bug1035074.patch

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

This is enough to solve this issue.

::: dom/messages/SystemMessageManager.js
@@ +218,5 @@
>              " and page URL = " + this._pageURL);
>        return;
>      }
>  
> +    if (null === this._dispatchers) {

Could you change this to "this._dispatchers === null"? I understand why you do this but we had better to follow the coding style in a consistent way.

@@ +220,5 @@
>      }
>  
> +    if (null === this._dispatchers) {
> +      // We hit the case that cpmm.removeMessageListener is unable to clean messages
> +      // which has already been dispatched. So just return.

Please just add a debug message for this:

debug("unit() has been called so |_dispatchers| is not available to dispatch messages.")

or something like this.
Attachment #8451580 - Flags: review+
(In reply to Henry Chang [:henry] from comment #8)
> Created attachment 8451580 [details] [diff] [review]
> Bug1035074.patch
> 
> Attached sample solution. Requires someone to verify the original bug.
> Jamin, could you please do me this favor? Thanks!

Per Tamara's comment, the error was fixed but bug 1030550 wasn't.
https://bugzilla.mozilla.org/show_bug.cgi?id=1030550#c9
(Assignee)

Comment 11

4 years ago
(In reply to Wesley Huang [:wesley_huang] from comment #10)
> (In reply to Henry Chang [:henry] from comment #8)
> > Created attachment 8451580 [details] [diff] [review]
> > Bug1035074.patch
> > 
> > Attached sample solution. Requires someone to verify the original bug.
> > Jamin, could you please do me this favor? Thanks!
> 
> Per Tamara's comment, the error was fixed but bug 1030550 wasn't.
> https://bugzilla.mozilla.org/show_bug.cgi?id=1030550#c9

I read the comments in bug 1030550 and it seems that callscreen received system message multiple times.
I wouldn't be surprised by this fact since callscreen created more than one SystemMessageManager
(looking for 'done: app://callscreen.gaiamobile.org/manifest.webapp' in the log), each of which 
was considered as a distinct event target because of different innerWindowID. 
SystemMessageInternal would store them in a list and dispatch messages to each of them.

I think the root cause might be the abnormal life cycle of callscreen. (init trice but uninit only once) I checked the code and callscreen
is only created once. Maybe the container of callscreen, AttentionScreen, doesn't manipulate callscreen properly.

[1] https://github.com/mozilla-b2g/gaia/blob/c086196d647ba35135061a7882657e8d63a8d1a8/apps/system/js/dialer_agent.js#L192
(In reply to Wesley Huang [:wesley_huang] from comment #10)
> (In reply to Henry Chang [:henry] from comment #8)
> > Created attachment 8451580 [details] [diff] [review]
> > Bug1035074.patch
> > 
> > Attached sample solution. Requires someone to verify the original bug.
> > Jamin, could you please do me this favor? Thanks!
Thanks. :)

I got the same result as Tamara did.
Please refer to https://bugzilla.mozilla.org/show_bug.cgi?id=1030550#c10.
Flags: needinfo?(jaliu)
(Assignee)

Comment 13

4 years ago
(In reply to Henry Chang [:henry] from comment #11)
> (In reply to Wesley Huang [:wesley_huang] from comment #10)
> > (In reply to Henry Chang [:henry] from comment #8)
> > > Created attachment 8451580 [details] [diff] [review]
> > > Bug1035074.patch
> > > 
> > > Attached sample solution. Requires someone to verify the original bug.
> > > Jamin, could you please do me this favor? Thanks!
> > 
> > Per Tamara's comment, the error was fixed but bug 1030550 wasn't.
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1030550#c9
> 
> I read the comments in bug 1030550 and it seems that callscreen received
> system message multiple times.
> I wouldn't be surprised by this fact since callscreen created more than one
> SystemMessageManager
> (looking for 'done: app://callscreen.gaiamobile.org/manifest.webapp' in the
> log), each of which 
> was considered as a distinct event target because of different
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> innerWindowID. 
^^^^^^^^^^^^^^^^
> SystemMessageInternal would store them in a list and dispatch messages to
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> each of them.
^^^^^^^^^^^^^^^

Something's wrong with this. Need more investigation and will come back later.
Henry: Gene: Is that normal that this patch doesn't have tests?

Updated

4 years ago
blocking-b2g: 2.0? → 2.0+
(Reporter)

Updated

4 years ago
Duplicate of this bug: 1031227
(Assignee)

Comment 16

4 years ago
Created attachment 8452998 [details] [diff] [review]
Bug10335074.patch

This is the root cause of this bug...
Attachment #8451580 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8452998 - Flags: review?(gene.lian)
(Assignee)

Comment 17

4 years ago
(In reply to Henry Chang [:henry] from comment #16)
> Created attachment 8452998 [details] [diff] [review]
> Bug10335074.patch
> 
> This is the root cause of this bug...

This patch may also fix bug 1030550. Will give it a tmr.
(Assignee)

Comment 18

4 years ago
(In reply to Henry Chang [:henry] from comment #17)
> (In reply to Henry Chang [:henry] from comment #16)
> > Created attachment 8452998 [details] [diff] [review]
> > Bug10335074.patch
> > 
> > This is the root cause of this bug...
> 
> This patch may also fix bug 1030550. Will give it a tmr.

Gave it a try and it fixed. Waiting for review.
(Assignee)

Updated

4 years ago
Whiteboard: [planned-sprint][in-sprint=v2.0-S5] → [planned-sprint][in-sprint=v2.0-S5][p=3][ft:ril]
(Reporter)

Comment 19

4 years ago
Comment on attachment 8452998 [details] [diff] [review]
Bug10335074.patch

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

This looks good to me. Thank you Henry! r=gene

This change is a critical one. I still hope Fernando or Fabrice can give it a look, who is the author/reviewer of this mechanism. Also, please ping him to see if we could have a test for this fix.
Attachment #8452998 - Flags: review?(gene.lian)
Attachment #8452998 - Flags: review?(ferjmoreno)
Attachment #8452998 - Flags: review+
(Reporter)

Comment 20

4 years ago
Comment on attachment 8452998 [details] [diff] [review]
Bug10335074.patch

Hi Fernando and Fabrice, could you please double check the fix? Either one first can take the review.

Summary of Henry's discover: this bug happens when a SystemMessageManager is still alive to receive messages from the parent process, even if the SystemMessageManager.uninit() has been invoked by the DOMRequestIpcHelper.destroyDOMRequestHelper(). The root cause is the SystemMessageManager instance is not completely released.
Attachment #8452998 - Flags: review?(fabrice)
(Assignee)

Comment 21

4 years ago
(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #20)
> Comment on attachment 8452998 [details] [diff] [review]
> Bug10335074.patch
> 
> Hi Fernando and Fabrice, could you please double check the fix? Either one
> first can take the review.
> 
> Summary of Henry's discover: this bug happens when a SystemMessageManager is
> still alive to receive messages from the parent process, even if the
> SystemMessageManager.uninit() has been invoked by the
> DOMRequestIpcHelper.destroyDOMRequestHelper(). The root cause is the
> SystemMessageManager instance is not completely released.

"SystemMessageManager instance is not completely released"
should be the "symptom". The root cause is we don't actually
remove the listener from cpmm while destroying DOMRequestHelper. 
The code to remove listeners should look like [1] but not [2].
(should check this._listeners[aName].weakRef)

I am going to mock a cpmm to test if correct version of remove(Weak)MessageListener is called.
Thanks!

[1] http://hg.mozilla.org/mozilla-central/file/fc35681b0a87/dom/base/DOMRequestHelper.jsm#l127
[2] http://hg.mozilla.org/mozilla-central/file/fc35681b0a87/dom/base/DOMRequestHelper.jsm#l189
Comment on attachment 8452998 [details] [diff] [review]
Bug10335074.patch

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

Ha yes, good catch!
Attachment #8452998 - Flags: review?(fabrice) → review+
Comment on attachment 8452998 [details] [diff] [review]
Bug10335074.patch

Thanks Henry!
Attachment #8452998 - Flags: review?(ferjmoreno) → review+
(Assignee)

Comment 24

4 years ago
Created attachment 8454554 [details] [diff] [review]
test_case.patch
(Assignee)

Comment 25

4 years ago
Attached preliminary test case. The try run https://tbpl.mozilla.org/?tree=Try&rev=802187c06ec0 failed because of this bug. 

:gene, :fabrice and :ferjm, what do you think of this test case? I am not able to have it tested with the patch at home and will be trying that next Monday. Thanks!
Flags: needinfo?(gene.lian)
(Assignee)

Updated

4 years ago
Flags: needinfo?(ferjmoreno)
Flags: needinfo?(fabrice)
Henry, I think you need to fix the tests ;)
Flags: needinfo?(fabrice)
(Assignee)

Comment 27

4 years ago
(In reply to Fabrice Desré [:fabrice] from comment #26)
> Henry, I think you need to fix the tests ;)

It failed just because I didn't apply the patch. It got passed 
after applying the patch so I am going to mark review flag for
test case.

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

Thanks!
(Assignee)

Updated

4 years ago
Attachment #8454554 - Flags: review?(gene.lian)
Attachment #8454554 - Flags: review?(ferjmoreno)
Attachment #8454554 - Flags: review?(fabrice)
Flags: needinfo?(gene.lian)
(Assignee)

Comment 28

4 years ago
Attached the test case and will merge with the original patch
before flagging to checkin-needed.
(Assignee)

Updated

4 years ago
Flags: needinfo?(ferjmoreno)
(Reporter)

Updated

4 years ago
Attachment #8454554 - Flags: review?(gene.lian) → review+
Attachment #8454554 - Flags: review?(ferjmoreno) → review+
Attachment #8454554 - Flags: review?(fabrice) → review+
(Assignee)

Comment 29

4 years ago
Created attachment 8455845 [details] [diff] [review]
Patch for checkin-needed (r+'d)

Thanks all of your review! This is the full try run https://tbpl.mozilla.org/?tree=Try&rev=b1f09b8293e7 and merged into one patch for checkin-needed. Thanks!
Attachment #8452998 - Attachment is obsolete: true
Attachment #8454554 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Updated

4 years ago
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
https://hg.mozilla.org/releases/mozilla-aurora/rev/2f78fcbe409e
status-b2g-v2.0: --- → fixed
status-b2g-v2.1: --- → fixed
status-firefox31: --- → wontfix
status-firefox32: --- → fixed
status-firefox33: --- → fixed
You need to log in before you can comment on or make changes to this bug.