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)
Tracking
()
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)
9.70 KB,
text/plain
|
Details | |
3.36 KB,
patch
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
> Please refer to bug 1030550, comment #c8 to reproduce this: ^^^^^^^^^^^^^^^^^^^^^^^^ I mean bug 1030550, comment #8
Reporter | ||
Updated•10 years ago
|
Summary: [system-message-api] SystemMessageManager will make JavaScript Error → [system-message-api] SystemMessageManager makes JS Error: "this._dispatchers is null"
Assignee | ||
Comment 4•10 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)
Assignee | ||
Comment 6•10 years ago
|
||
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•10 years ago
|
||
Attachment #8451563 -
Attachment is obsolete: true
Assignee | ||
Comment 8•10 years ago
|
||
Attached sample solution. Requires someone to verify the original bug. Jamin, could you please do me this favor? Thanks!
Flags: needinfo?(jaliu)
Updated•10 years ago
|
Component: General → DOM: Device Interfaces
Product: Firefox OS → Core
Version: unspecified → 32 Branch
Reporter | ||
Comment 9•10 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+
Comment 10•10 years ago
|
||
(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•10 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
Comment 12•10 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! 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•10 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.
Comment 14•10 years ago
|
||
Henry: Gene: Is that normal that this patch doesn't have tests?
Updated•10 years ago
|
blocking-b2g: 2.0? → 2.0+
Assignee | ||
Comment 16•10 years ago
|
||
This is the root cause of this bug...
Attachment #8451580 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8452998 -
Flags: review?(gene.lian)
Assignee | ||
Comment 17•10 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•10 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•10 years ago
|
Whiteboard: [planned-sprint][in-sprint=v2.0-S5] → [planned-sprint][in-sprint=v2.0-S5][p=3][ft:ril]
Reporter | ||
Comment 19•10 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•10 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•10 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 22•10 years ago
|
||
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 23•10 years ago
|
||
Comment on attachment 8452998 [details] [diff] [review] Bug10335074.patch Thanks Henry!
Attachment #8452998 -
Flags: review?(ferjmoreno) → review+
Assignee | ||
Comment 24•10 years ago
|
||
Assignee | ||
Comment 25•10 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•10 years ago
|
Flags: needinfo?(ferjmoreno)
Flags: needinfo?(fabrice)
Assignee | ||
Comment 27•10 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•10 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•10 years ago
|
||
Attached the test case and will merge with the original patch before flagging to checkin-needed.
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(ferjmoreno)
Reporter | ||
Updated•10 years ago
|
Attachment #8454554 -
Flags: review?(gene.lian) → review+
Updated•10 years ago
|
Attachment #8454554 -
Flags: review?(ferjmoreno) → review+
Updated•10 years ago
|
Attachment #8454554 -
Flags: review?(fabrice) → review+
Assignee | ||
Comment 29•10 years ago
|
||
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•10 years ago
|
Keywords: checkin-needed
Reporter | ||
Comment 30•10 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/42a4c5fcc510
Keywords: checkin-needed
Assignee | ||
Updated•10 years ago
|
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
Target Milestone: --- → mozilla33
Comment 32•10 years ago
|
||
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.
Description
•