Closed Bug 1001348 Opened 10 years ago Closed 10 years ago

Preserve system sources when certified debugging is allowed

Categories

(Firefox OS Graveyard :: General, defect)

Product:
Firefox OS Graveyard
Component:
General
Platform:
x86_64
macOS
Type:
defect
Priority:
Not set
Severity:
major

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
2.0 S1 (9may)

People

(Reporter: TYLin, Assigned: ochameau)

References

Details

(Whiteboard: [preserving sources breaks multi core devices, see comment 53])

Attachments

(2 files, 1 obsolete file)

Keep system sources when certified debugging is allowed
1.48 KB, patch
vingtetun
: review+
Details | Diff | Splinter Review
Disable discardSystemSources when using DEVICE_DEBUG=1 from gaia
46 bytes, text/x-github-pull-request
timdream
: review+
jryans
: feedback+
Details | Review 
I found this issue while flashing gecko on flame device.
It works for me on latest master/m-c builds on a nexus-s. I tried to debug the calculator app and I could see the source, set breakpoints, etc. Do you get any errors in logcat or in the browser console?
Summary: Debugger show [no source] in App Manager → Debugger show [no source] when debugging built-in apps in App Manager
I found this issue when debugging built-in keyboard app. After searching for recent patches, I notice that bug 990353 discard the js sources for chrome and build-in apps. After changing the pref "javascript.options.discardSystemSource" to false, the source code reappears. http://hg.mozilla.org/mozilla-central/rev/744e3fa4bb8a
Depends on: 990353
Yeah. Is this something that users are likely to do? Maybe the debugger should automatically flip this pref or something?
It looks like javascript.options.discardSystemSource only affect system apps and chrome.
In devtools world, we have devtools.debugger.forbid-certified-apps which is set to true by default
and prevent debugging system apps and chrome... so I'm wondering if we can just dynamically flip the js pref from devtools code, once the developer set forbid-certified-apps to false?
+1. It will be convenient if developer tool could flip javascript.options.discardSystemSource when devtools.debugger.forbid-certified-apps is set to true.
How can we implement that? Somewhere in the startup code we check if forbid-certified-apps is false, then switch the discardSystemSource pref?
Sounds pretty aggressive to me. Many developers have forbid-certified-apps set to false. Might be confusing… But I don't see any better way to solve that.
I'll give this a shot.  It's pretty confusing as is.
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Component: Developer Tools: App Manager → General
Product: Firefox → Firefox OS
Version: Trunk → unspecified
Summary: Debugger show [no source] when debugging built-in apps in App Manager → Preserve system sources when certified debugging is allowed
Set the discardSystemSources pref at startup based on the state of the forbid-certified-apps pref.  I've tested this locally with a simulator build.
Attachment #8415439 - Flags: review?(21)
Because this is set dynamically, the main process will discard the sources the first time it starts without this pref. A B2G restart would be required. It's not a real issue though.
(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #14)
> Because this is set dynamically, the main process will discard the sources
> the first time it starts without this pref. A B2G restart would be required.
> It's not a real issue though.

Yeah, that's true...  Someone might get confused, but we can just tell them to restart. :)
What is the ETA for this. It's been several days I haven't been able to use the debugger. It's my bred and butter.
Comment on attachment 8415439 [details] [diff] [review]
Keep system sources when certified debugging is allowed

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

\o/
Attachment #8415439 - Flags: review?(21) → review+
(In reply to Diego Marcos [:dmarcos] from comment #16)
> What is the ETA for this. It's been several days I haven't been able to use
> the debugger. It's my bred and butter.

If you need a quicker fix, you can set the pref yourself, as that is all that this patch does anyway.
I've tried setting both prefs to false, restarted nightly and it doesn't seem to do any difference. I still don't see any sources

I tried 

devtools.debugger.forbid-certified-apps = false
javascript.options.discardSystemSource = false
Flags: needinfo?(jryans)
(In reply to Diego Marcos [:dmarcos] from comment #19)
> I've tried setting both prefs to false, restarted nightly and it doesn't
> seem to do any difference. I still don't see any sources
> 
> I tried 
> 
> devtools.debugger.forbid-certified-apps = false
> javascript.options.discardSystemSource = false

Did you set these prefs on the phone / in the simulator?  When you say "restarted nightly", it makes me think you set them in desktop Firefox...
Flags: needinfo?(jryans)
I added a custom-prefs.js file to my gaia checkout with the following:

user_pref("devtools.debugger.forbid-certified-apps", false);
user_pref("javascript.options.discardSystemSource", false);

I did make reset-gaia and with the 2nd option gaia doesn't boot. I get a screen with home button but no homescreen, just a black background. If I remove user_pref("javascript.options.discardSystemSource", false); gaia starts up normally.
Flags: needinfo?(jryans)
(In reply to Diego Marcos [:dmarcos] from comment #23)
> I added a custom-prefs.js file to my gaia checkout with the following:
> 
> user_pref("devtools.debugger.forbid-certified-apps", false);
> user_pref("javascript.options.discardSystemSource", false);
> 
> I did make reset-gaia and with the 2nd option gaia doesn't boot. I get a
> screen with home button but no homescreen, just a black background. If I
> remove user_pref("javascript.options.discardSystemSource", false); gaia
> starts up normally.

Okay, I concur with your findings.  I assumed for this type of change that testing on the Simulator would be enough (since it was also affected), but it seems not. :(

On my Nexus 4, I can get to the homescreen background, but no app icons appear if "javascript.options.discardSystemSource" is "false".

I've asked KWierso to back out the change for now.

bholley, any idea what's wrong here?  I don't see why keeping system sources now seems to break Gaia's UI entirely.
Flags: needinfo?(jryans) → needinfo?(bobbyholley)
(In reply to J. Ryan Stinnett [:jryans] from comment #25)
> bholley, any idea what's wrong here?  I don't see why keeping system sources
> now seems to break Gaia's UI entirely.

That seems bad! Please debug it, and let us know what's going on.
Flags: needinfo?(bobbyholley)
(In reply to Bobby Holley (:bholley) from comment #26)
> (In reply to J. Ryan Stinnett [:jryans] from comment #25)
> > bholley, any idea what's wrong here?  I don't see why keeping system sources
> > now seems to break Gaia's UI entirely.
> 
> That seems bad! Please debug it, and let us know what's going on.

I don't think I have enough context to debug this efficiently... I mostly stopped by to set a pref that you and others suggested could be used to make chrome scripts appear again for debugging.

It now appears that setting the pref to "false" (which should bring us back to how things used to work) has strange side effects on the Gaia UI.  I don't see any obvious errors in the logs when this happens.

Bobby, is there anyone with more context about how the chrome script source discarding functions that can take a look at this?  Fabrice, possibly you can help here?
Flags: needinfo?(fabrice)
Flags: needinfo?(bobbyholley)
I think we should back out the patch that first broke debugging for certified apps. It's been a week I haven't been able to work properly. Do you know what is the bug number?
Flags: needinfo?(jryans)
(In reply to Diego Marcos [:dmarcos] from comment #28)
> I think we should back out the patch that first broke debugging for
> certified apps. It's been a week I haven't been able to work properly. Do
> you know what is the bug number?

It's bug 990353.  Bobby and Fabrice were involved in this original bug, so that's why I am hoping they can help here / advise how to proceed.

I agree that it's quite frustrating at this time to work on certified apps since debugging does not work.
Flags: needinfo?(jryans)
(In reply to J. Ryan Stinnett [:jryans] from comment #27)
> It now appears that setting the pref to "false" (which should bring us back
> to how things used to work) has strange side effects on the Gaia UI.  I
> don't see any obvious errors in the logs when this happens.

Are you setting this pref at boot time? Twiddling the pref at Runtime may not work correct.

(In reply to Diego Marcos [:dmarcos] from comment #28)
> I think we should back out the patch that first broke debugging for
> certified apps.

That's unlikely to be helpful. You can turn off the behavior here with a pref. And if that doesn't do the job, that means that there have been gaia changes in the interim that mean that gaia won't run _without_ discarding (which would be surprising, but who knows?)

> Bobby, is there anyone with more context about how the chrome script source
> discarding functions that can take a look at this?  Fabrice, possibly you
> can help here?

The situation as I understand it as follows:
* If the gaia problems only occur when twiddling this pref at runtime, then we understand the issue, and the answer is to flip the pref and reboot.
* If the gaia problems persist even after setting the pref and rebooting, then this is a gaia bug, and needs to be investigated by someone who understands that stack.
Flags: needinfo?(bobbyholley)
https://hg.mozilla.org/mozilla-central/rev/189a0e19896f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S1 (9may)
I don't understand. Why is this marked as fixed? The patch look the same as the one we backed out.
Flags: needinfo?(kwierso)
(In reply to Diego Marcos [:dmarcos] from comment #32)
> I don't understand. Why is this marked as fixed? The patch look the same as
> the one we backed out.

I'll reopen, looks like only the original patch made it over to m-c.  The backout will come over on the next merge to m-c.  Hopefully that's soon...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Bobby Holley (:bholley) from comment #30)
> (In reply to J. Ryan Stinnett [:jryans] from comment #27)
> > It now appears that setting the pref to "false" (which should bring us back
> > to how things used to work) has strange side effects on the Gaia UI.  I
> > don't see any obvious errors in the logs when this happens.
> 
> Are you setting this pref at boot time? Twiddling the pref at Runtime may
> not work correct.

While the original patch attached here changed it at runtime, Diego and I have each tried setting it at boot time (in the Gaia profile's prefs), and it still results in a broken Gaia UI.
(In reply to J. Ryan Stinnett [:jryans] from comment #34)
> While the original patch attached here changed it at runtime, Diego and I
> have each tried setting it at boot time (in the Gaia profile's prefs), and
> it still results in a broken Gaia UI.

Ok. Given that the pref is the only thing that differentiates the b2g behavior from the Desktop / pre bug 990353 behavior, this sounds like a Gaia bug. You can try backing out bug 990353 from trunk in a local build to confirm that the problem persists.
Ting-Yu had success with the pref in comment 3.

I'll check tomorrow but just saying "gaia doesn't boot" without providing a logcat is not really actionable for us.
Flags: needinfo?(fabrice)
(In reply to Fabrice Desré [:fabrice] from comment #36)
> Ting-Yu had success with the pref in comment 3.

Ok, that implies that the regression happened sometime after April 27th.
(In reply to Bobby Holley (:bholley) from comment #35)
> (In reply to J. Ryan Stinnett [:jryans] from comment #34)
> > While the original patch attached here changed it at runtime, Diego and I
> > have each tried setting it at boot time (in the Gaia profile's prefs), and
> > it still results in a broken Gaia UI.
> 
> Ok. Given that the pref is the only thing that differentiates the b2g
> behavior from the Desktop / pre bug 990353 behavior, this sounds like a Gaia
> bug. You can try backing out bug 990353 from trunk in a local build to
> confirm that the problem persists.

I've now backed out the patches from bug 990353 locally and Gaia works correctly, so the patches in bug 990353 do seem to contribute to the problem in some way.

While backing them out, I had to work around a conflict with other changes from later patches from bug 987556.  I'll now try a local build by re-applying bug 990353 and removing bug 987556 to see if there's some kind of interaction between them.
(In reply to J. Ryan Stinnett [:jryans] from comment #38)
> While backing them out, I had to work around a conflict with other changes
> from later patches from bug 987556.  I'll now try a local build by
> re-applying bug 990353 and removing bug 987556 to see if there's some kind
> of interaction between them.

Removing bug 987556 (with bug 990353 still applied) did not seem to have an effect, as Gaia is still broken.

The next step will be to roll back to just after bug 990353 landed and check the state there.
(In reply to Fabrice Desré [:fabrice] from comment #36)
> Ting-Yu had success with the pref in comment 3.
> 
> I'll check tomorrow but just saying "gaia doesn't boot" without providing a
> logcat is not really actionable for us.

I've attached my logcat output from the case where the homescreen appears but shows no app icons (which happens when setting the pref to keep system sources (discardSystemSource == false).
I've now tested locally rolling back to just after bug 990353 originally landed (Apr 22), and the Gaia UI is still broken for me when discardSystemSource == false.

Perhaps behavior is different for different devices...? :/ I've been testing with a Nexus 4.

Diego, what device are you using?
Flags: needinfo?(dmarcos)
To clarify, I did not yet test older Gaia, only different Gecko versions.
(In reply to Diego Marcos [:dmarcos] from comment #32)
> I don't understand. Why is this marked as fixed? The patch look the same as
> the one we backed out.

Sorry, the bug resolution script we run when we merge trees around don't know about things that get backed out outside of the items within the merge, so it automatically resolved it for me.
I'm using a Nexus 4 as well. It's the only device I can use for the camera specific stuff I'm working on. I need touch to focus and face tracking available on the hardware side.
Flags: needinfo?(dmarcos)
(In reply to J. Ryan Stinnett [:jryans] from comment #42)
> I've now tested locally rolling back to just after bug 990353 originally
> landed (Apr 22), and the Gaia UI is still broken for me when
> discardSystemSource == false.

Using both Gecko & Gaia from Apr 22 (after bug 990353 originally landed) with discardSystemSource == false, the Nexus 4's homescreen is still broken.  It seems likely so far that at least this device has never worked correctly with "discardSystemSource == false" since the bug was landed.

I'll now look into what happens on the Keon.
There is nothing device-specific in these patches. You seem to only bisect gecko, I would also bisect gaia. Just testing with daily builds and overriding the pref should help a lot.
(In reply to Fabrice Desré [:fabrice] from comment #47)
> There is nothing device-specific in these patches. You seem to only bisect
> gecko, I would also bisect gaia. Just testing with daily builds and
> overriding the pref should help a lot.

It probably wasn't very clear as I've made many comments now...  but in my last comment 46, I did reset *both* Gecko to Apr 22 and Gaia to Apr 22.  Gaia still presents a broken homescreen when discardSystemSource is false in this build for the Nexus 4.

I've now tested on a Keon with today's nightly build where I set discardSystemSource to false.  In this configuration, Gaia works just fine, and system sources are preserved as the pref's value says they should be.

So, it seems pretty likely that this *is* specific to the Nexus 4.  I only have a Nexus 4 and Keon, so I can't test other devices to report on their status.

Fabrice, is there other bisecting that would help here?  To me, it seems clear now the pref was broken when it landed, but perhaps only on the Nexus 4.
Flags: needinfo?(fabrice)
Whiteboard: [preserving sources seems to break nexus 4, see comment 48]
Well, that very strange but I don't have any other ideas. If other people could try with other devices that may help narrow this down...

The only difference I can think of is that the keon is single core while the nexus 4 is not.
Flags: needinfo?(fabrice)
I believe there is or was some kind of difference in source code handling depending on the number of cores.  I think source compression isn't used on a single core device?
(In reply to Andrew McCreight [:mccr8] from comment #50)
> I believe there is or was some kind of difference in source code handling
> depending on the number of cores.  I think source compression isn't used on
> a single core device?

That's correct.
(In reply to Fabrice Desré [:fabrice] from comment #51)
> (In reply to Andrew McCreight [:mccr8] from comment #50)
> > I believe there is or was some kind of difference in source code handling
> > depending on the number of cores.  I think source compression isn't used on
> > a single core device?
> 
> That's correct.

I'll try a Nexus 4 build with source compression disabled.  Is it setting this bool[1] to false sufficient to correctly disable it on multi-core?

[1]: http://dxr.mozilla.org/mozilla-central/source/js/src/jsscript.cpp#1603
Okay, it does appear to be related to source compression!  By disabling source compression on the Nexus 4, discardSystemSource = false still allows Gaia to start up correctly.

I'll file a new JS engine bug that this can depend on to work out the underlying issue here.
Whiteboard: [preserving sources seems to break nexus 4, see comment 48] → [preserving sources breaks multi core devices, see comment 53]
Can we pref off this feature on m-c? This feature was first introduced because of the lowmem phone. We have not try to make it work on m-c yet.
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #54)
> Can we pref off this feature on m-c? This feature was first introduced
> because of the lowmem phone. We have not try to make it work on m-c yet.

I was thinking about tweaking DEVICE_DEBUG, to ease things around this pref.
It will prevent having to reboot and so the sources appear on very first launch.
But it won't address Nexus4 issue at all, instead it will prevent using DEVICE_DEBUG=1 on Nexus, until bug 1006695 is fixed.

Your thoughts?
Attachment #8419295 - Flags: review?(timdream)
Comment on attachment 8419295 [details] [review]
Disable discardSystemSources when using DEVICE_DEBUG=1 from gaia

This can work but I am not entirely sure this solves the use cases here. If others agree we can land this as the patch to this bug.
Attachment #8419295 - Flags: review?(timdream)
Attachment #8419295 - Flags: review+
Attachment #8419295 - Flags: feedback?(dmarcos)
Comment on attachment 8419295 [details] [review]
Disable discardSystemSources when using DEVICE_DEBUG=1 from gaia

I think Alex's patch here is a better way to handle the issue, since we know for sure the pref will be set before boot time.

I would suggest waiting until a fix is landed for bug 1006695 to avoid breaking people who use multi-core devices like the Nexus.  There is now a working patch attached there, so hopefully we're not too far from having it fixed.
Attachment #8419295 - Flags: feedback+
We care about memory on 256 MiB devices too, and the wins on those devices are even higher (over 4MiB) than they are on Tarako (because we already minify system sources on Tarako). I don't think we should give that up if we can avoid it.
Is there any known workaround for Flame or Hamachi until this is resolved? Are there some prefs I can set inside gaia? (I don't keep a local copy of m-c, just gaia)
(In reply to Jared Hirsch [:_6a68] [@6a68] from comment #59)
> Is there any known workaround for Flame or Hamachi until this is resolved?
> Are there some prefs I can set inside gaia? (I don't keep a local copy of
> m-c, just gaia)

If you are using a single-core device (I believe Hamachi is single core...?), then you only need to set the pref "javascript.options.discardSystemSource" to "false".

If you are using a multi-core device (Flame, Nexus 4, etc.), then I am only aware of fixes that require recompiling Gecko first (until bug 1006695 is fixed).  You could rollback Gecko to a build from Apr 22 or earlier for now, if that is easier for you than applying a patch and building locally.
(In reply to Jared Hirsch [:_6a68] [@6a68] from comment #59)
> Is there any known workaround for Flame or Hamachi until this is resolved?
> Are there some prefs I can set inside gaia? (I don't keep a local copy of
> m-c, just gaia)

If you can live with an old gecko and have access to PVT builds. 20140422160202 is the latest build I found where on device debugging still works. You can use the flash tool (https://github.com/Mozilla-TWQA/B2G-flash-tool) Downgrade gecko/gaia and then you can flash your gaia on top. 

./auto_flash_from_pvt.sh -d nexus4 -b 20140422160202
Thanks :jryans and :dmarcos! I had tried the previous day's pvt build (2014-04-21), but it threw errors and couldn't properly attach devtools. 20140422160202 + gaia HEAD works great for me on Hamachi :-)
Is there any ETA for landing this patch?
(In reply to Jared Hirsch [:_6a68] [@6a68] from comment #63)
> Is there any ETA for landing this patch?

This patch is currently waiting on the work in bug 1006695 to be completed (to avoid breaking multi-core devices).  If you need to use a newer build now (with a single-core device), it's best to set the pref yourself.
Thanks for the info! I'll watch that bug and attempt to build gecko myself in the meantime :-)
Alex, I think it's safe to land your fix now that the multi-core bug is resolved.  What do you think?

I'm leaving on PTO soon, so I'll assign this bug to you.
Assignee: jryans → poirot.alex
Flags: needinfo?(poirot.alex)
I have verified on my Nexus 4 that it is now safe to set discardSystemSource = false.
Ok so since it is safe to enable discardSystemSource I think we can now proceed and land both patches.
The gaia patch will also to see sources immediately, without reboot, when using DEVICE_DEBUG=1 while building gaia.
The gecko patch will also regular app developers (and gaia devs that aren't using DEVICE_DEBUG) to see sources after rebooting at least once after having toggled the forbid-certified-apps pref.
Flags: needinfo?(poirot.alex)
New try for gecko patch, just to be sure:
  https://tbpl.mozilla.org/?tree=Try&rev=9e0629cf0369
Try is green.
I got a failure on G-oop, but it seems to be due to the changeset I'm based on,
as I get the exact same here for another patch:
  https://tbpl.mozilla.org/?tree=Try&rev=036f06b17e4d
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/dc386dd7700e
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Attachment #8419295 - Flags: feedback?(dmarcos)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: