Closed Bug 998898 Opened 5 years ago Closed 5 years ago

Can't open the webconsole for apps

Categories

(DevTools :: WebIDE, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 31

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

(Keywords: regression, Whiteboard: [needs-coverage])

Attachments

(1 file, 4 obsolete files)

Looks like I regressed apps debugging in bug 997239.
I would say that the special behavior of globals on b2g makes it so that webbrowser.js doesn't pull `devtools` symbol from main.js scope (whereas it 
does on desktop...)

We can see the following exception, in firefox, when clicking on DEBUG:
Server did not specify an actor, dropping packet: {"error":"unknownError","message":"error occurred while processing 'attach: ReferenceError: devtools is not defined\nStack: DebuggerProgressListener@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/actors/webbrowser.js:1264
Attached patch Fix webbrowser dependencies (obsolete) — Splinter Review
I have limited ressources today.
I haven't had a chance to even build this patch yet,
but the exception looks quite obvious.
There is another regression against the simulator that I faced while testing the previous patch:
Server did not specify an actor, dropping packet: {"error":"unknownError","message":"error occurred while processing 'startListeners: TypeError: mm.addMessageListener is not a function\nStack: NetworkMonitorChild.prototype.init@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/toolkit/webconsole/network-monitor.js:1047:1\nWCA_onStartListeners@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/webconsole.js:553:13\nDSC_onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js:1275:9\nChildDebuggerTransport.prototype.receiveMessage@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/transport.js:347:5\nLine: 1047, column: 0"}

Bug 989043 most likely introduced that, by assuming that chromeEventHandler is the messageManager,
which is true only with e10s frames where TabChild is both the chrome event handler and the message manager.
It happen to work on e10s firefox as messageManager is overloaded in BrowserTabActor.
May be I should also remove this overload as this patch make it useless.
Attached patch Fix webbrowser dependencies (obsolete) — Splinter Review
Just realized that these patches depends on other patches of my queue.
Here is same patches that applies as-is on tip.
Attachment #8409600 - Attachment is obsolete: true
Attachment #8409652 - Attachment is obsolete: true
Assignee: nobody → poirot.alex
Blocks: 989043, 997239
Attachment #8409657 - Flags: review?(jryans)
Attachment #8409658 - Flags: review?(jryans)
Keywords: regression
Whiteboard: [needs-coverage]
Comment on attachment 8409657 [details] [diff] [review]
Fix webbrowser dependencies

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

::: toolkit/devtools/server/actors/webbrowser.js
@@ +14,4 @@
>  let {Promise: promise} = Cu.import("resource://gre/modules/Promise.jsm", {});
>  XPCOMUtils.defineLazyModuleGetter(this, "AddonManager", "resource://gre/modules/AddonManager.jsm");
>  
> +// Also depends on following symbols, shared by common scope with main.js:

Is it really safe to depend on these symbols like this?  See also bug 998912, where it is clear they are not always implicitly available like you are assuming.

We should really strive for some kind of explicit dependence here, and in the other places where you've made the same assumption.
Attachment #8409657 - Flags: review?(jryans)
Priority: -- → P1
Comment on attachment 8409658 [details] [diff] [review]
Fix webconsole exception on simulator

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

r+ but yes, please also clean up the override in BrowserTabActor as well.
Attachment #8409658 - Flags: review?(jryans) → review+
I saw similar error when debug built-in keyboard or homescreen app.

onPacket threw an exception: Error: Server did not specify an actor, dropping packet: {"error":"unknownError","message":"error occurred while processing 'attach: ReferenceError: devtools is not defined\nStack: DebuggerProgressListener@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/actors/webbrowser.js:1264:1\nBTA_attach@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/actors/webbrowser.js:676:5\nBTA_onAttach@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/actors/webbrowser.js:744:5\nDSC_onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js:1185:9\nChildDebuggerTransport.prototype.receiveMessage@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/transport.js:347:5\nLine: 1264, column: 0"}
TYLin, the patch introducing this regression has been backed out.
Please update your gecko to the very last tip.

I'll morph this bug into just fixing the console, which has been broken by bug 989043.
Summary: Can't debug apps → Can't open the webconsole for apps
Attachment #8409657 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/8685f4d027ea
Keywords: checkin-needed
Whiteboard: [needs-coverage] → [needs-coverage][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/8685f4d027ea
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [needs-coverage][fixed-in-fx-team] → [needs-coverage]
Target Milestone: --- → Firefox 31
What does the [needs-coverage] whiteboard tag mean? Does that signify this needs tests in-testsuite? does it need manual testing from QA? both? something else?
Flags: needinfo?(jryans)
(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #13)
> What does the [needs-coverage] whiteboard tag mean? Does that signify this
> needs tests in-testsuite? does it need manual testing from QA? both?
> something else?

I've been using it to mark specific issues that are either not covered or poorly covered by existing tests that we'd like to improve.  I guess it's similar to in-testsuite-, but sometimes bugs still have *some* tests but still not *enough*. :)

Anyway, it does not mean the QA team needs to do anything.  Sorry for any confusion.
Flags: needinfo?(jryans)
Thanks for the explanation :jryans. I'm going to tag this [qa-] since you don't need QA on this bug. If that changes please need-info me.
QA Whiteboard: [qa-]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.