Closed Bug 941012 Opened 10 years ago Closed 10 years ago

Firefox OS 1.2 Simulator is not connected


(DevTools Graveyard :: WebIDE, defect)

Windows 7
Not set


(Not tracked)

Firefox 28


(Reporter: phorea, Assigned: ochameau)



(Whiteboard: [simulator][good first verify][needs-coverage])


(2 files, 4 obsolete files)

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0

Reproducible on latest Beta Fx 26 beta 6 (20131118212339), latest Aurora (20131120004002), latest Nightly (20131120030202).

Steps to reproduce:
1. Open App Manager
2. Start the Firefox 1.2 OS Simulator

Actual Results:
"Connecting to.." is displayed on the bottom bar for a few seconds, the Simulator window is opened then in the console "disconnected" is shown. The string on the bottom bar returns to "Not Connected. No device found. Plug a device or Start Simulator".
The window remains open.

Expected Results:
The Simulator is connected, on the console the "connected" string is displayed.

Notes: - We didn't encounter this issue using Firefox OS 1.3 Simulator
- It is not reproducible when using the app manager for the first time 
- Intermittently the issue reproduces also on MAC OS X and Ubuntu 13.10
Whiteboard: [simulator]
More or less a revert of bug 911127, but fixed against b2g.
It end up fixing this issue, where DebuggerServer fails to load on 1.2,
except on first run (!!).

The loadSubScript code is quite complex and end up having same issue
than when using SDK loader on non-first run.

I haven't looked at precisely why, but, on b2g, main.js sandbox
has a b2g-jsm like behavior. Its global object is different than `this`
in top level scope. So that, when we do `var DebuggerServer = ...`
in top level scope, it isn't set on `this`. (i.e. this.DebuggerServer is undefined)

That's why we had the sdk loader exception, even with:
'({components : Components}) = require("chrome");
It won't overload this.Components, so that transport.js will still use sdk loader Components.utils.

We can fix that by doing:
Object.definedProperty(this, "Components", {get: function () require("chrome").components});
(defineProperty, as SDK loader se definedProperties to set Components.)

Then, the bad part of this patch is that we have to do similar trick for all
symbols used by scripts loaded with loadSubScript (DebuggerServer, dumpn, SourceMap*,...).
I haven't exported escodegen as I haven't seen any usage??
(ideally, all dependencies would explicitely load what it uses!)
Assignee: nobody → poirot.alex
Comment on attachment 8336446 [details] [diff] [review]
Always use SDK loader to load DebuggerServer r=dcamp

Review of attachment 8336446 [details] [diff] [review]:

Now that I've looked at the patch, this change is clearer than when we talked earlier, so I am fine with reviewing this.

The try results suggest to me that you probably do need to export escodegen, so take a look at that.

Also, please test the original use case that we changed this to support, namely if you start the browser debugger, ensure you are able to debug an actor.

r+ assuming you look into those things.
Attachment #8336446 - Flags: review+
Do we need to uplift that to beta?
Yes we will need uplift. I volontary kept this patch short and simple.
But be carefull, I only tested that on b2g-simulator master,
that's why I haven't asked for the review yet.
Attached patch 1.2 patch (obsolete) — Splinter Review
I fixed the escodegen error and rebased it on 1.2.
Comment on attachment 8338451 [details] [diff] [review]
master patch - fixed ChromeWorker exception

Review of attachment 8338451 [details] [diff] [review]:

My tree doesn't want to compile for b2g on device, could you give a try to this patch and bless it again?
I added two additional fixes after your review.
1) rebases the patch against last master that doesn't uses escodegen anymore (and fixed escodegen in 1.2 patch)
2) Injects ChromeWorker in all devtools sdk modules (I need to update 1.2 patch accordingly)

[tbpl is now green]
Attachment #8338451 - Flags: review?(jryans)
And TBPL run:
Attachment #8337833 - Attachment is obsolete: true
(In reply to Alexandre Poirot (:ochameau) from comment #10)
> Created attachment 8338516 [details] [diff] [review]
> 1.2 patch - with ChromeWorker fix
> And TBPL run:

Seems like this one still has some escodegen issues.
Comment on attachment 8338451 [details] [diff] [review]
master patch - fixed ChromeWorker exception

Review of attachment 8338451 [details] [diff] [review]:

Looks good.  I've tested this locally on my Keon with 1.3 (2013-11-26), and it seems to work well.
Attachment #8338451 - Flags: review?(jryans) → review+
Checking needed for attachment 8338451 [details] [diff] [review] on master.
Keywords: checkin-needed
Keywords: checkin-needed
Whiteboard: [simulator] → [simulator][fixed-in-fx-team]
This was backed out and re-landed while investigating a test failure.
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [simulator][fixed-in-fx-team] → [simulator]
Target Milestone: --- → Firefox 28
Whiteboard: [simulator] → [simulator][good first verify]
Whiteboard: [simulator][good first verify] → [simulator][good first verify][needs-coverage]
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.