Closed Bug 1035074 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: DOM: Device Interfaces, defect)

32 Branch
ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33
blocking-b2g 2.0+
Tracking Status
firefox31 --- wontfix
firefox32 --- fixed
firefox33 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: airpingu, Assigned: hchang)

References

Details

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

Attachments

(2 files, 4 obsolete files)

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.
> Please refer to bug 1030550, comment #c8 to reproduce this:
                  ^^^^^^^^^^^^^^^^^^^^^^^^ I mean bug 1030550, comment #8
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)
(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)
assign to Henry then, thanks!
Assignee: nobody → hchang
Attached file diagnostic.txt (obsolete) —
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|.
Attached file diagnostic.txt
Attachment #8451563 - Attachment is obsolete: true
Attached patch Bug1035074.patch (obsolete) — Splinter Review
Attached sample solution. Requires someone to verify the original bug. Jamin, could you please do me this favor? Thanks!
Flags: needinfo?(jaliu)
Component: General → DOM: Device Interfaces
Product: Firefox OS → Core
Version: unspecified → 32 Branch
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
(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)
(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?
blocking-b2g: 2.0? → 2.0+
Attached patch Bug10335074.patch (obsolete) — Splinter Review
This is the root cause of this bug...
Attachment #8451580 - Attachment is obsolete: true
Attachment #8452998 - Flags: review?(gene.lian)
(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.
(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.
Whiteboard: [planned-sprint][in-sprint=v2.0-S5] → [planned-sprint][in-sprint=v2.0-S5][p=3][ft:ril]
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+
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)
(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+
Attached patch test_case.patch (obsolete) — Splinter Review
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)
Flags: needinfo?(ferjmoreno)
Flags: needinfo?(fabrice)
Henry, I think you need to fix the tests ;)
Flags: needinfo?(fabrice)
(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!
Attachment #8454554 - Flags: review?(gene.lian)
Attachment #8454554 - Flags: review?(ferjmoreno)
Attachment #8454554 - Flags: review?(fabrice)
Flags: needinfo?(gene.lian)
Attached the test case and will merge with the original patch
before flagging to checkin-needed.
Flags: needinfo?(ferjmoreno)
Attachment #8454554 - Flags: review?(gene.lian) → review+
Attachment #8454554 - Flags: review?(ferjmoreno) → review+
Attachment #8454554 - Flags: review?(fabrice) → review+
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
Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: