1014141 - Regression in the remote debugger

Status: RESOLVED FIXED

(Firefox Graveyard :: Webapp Runtime, defect, P1)

Description

[Marco Castelluccio [:marco]]

The error is: "ReferenceError: BrowserTabList is not defined".
The problem is most likely the same as bug 1010750.

Comment 1

[Marco Castelluccio [:marco]]

Since bug 859372 regressed this feature, could you take this bug?

Comment 2

[Eddy Bruel [:ejpbruel]]

Sure. I'll look into this on monday. Sorry for breaking it!

Comment 3

[Marco Castelluccio [:marco]]

I guess this regression is going into Aurora now?
I think we should uplift the fix as soon as it's ready.

Comment 4

[Eddy Bruel [:ejpbruel]]

(In reply to Marco Castelluccio [:marco] from comment #3)
> I guess this regression is going into Aurora now?
> I think we should uplift the fix as soon as it's ready.

From what I can tell, this bug is a duplicate of bug 1010750, and should have been fixed when that bug got fixed.

Marco, can you confirm this for me?

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Eddy Bruel [:ejpbruel] from comment #4)
> (In reply to Marco Castelluccio [:marco] from comment #3)
> > I guess this regression is going into Aurora now?
> > I think we should uplift the fix as soon as it's ready.
> 
> From what I can tell, this bug is a duplicate of bug 1010750, and should
> have been fixed when that bug got fixed.
> 
> Marco, can you confirm this for me?

I haven't tested, but I don't believe it's fixed...  The WebappRT code path[1] likely needs similar fixes as were needed for Fennec in bug 1010750.

[1]: http://dxr.mozilla.org/mozilla-central/source/webapprt/RemoteDebugger.jsm#22

Comment 6

[Eddy Bruel [:ejpbruel]]

(In reply to J. Ryan Stinnett [:jryans] from comment #5)
> (In reply to Eddy Bruel [:ejpbruel] from comment #4)
> > (In reply to Marco Castelluccio [:marco] from comment #3)
> > > I guess this regression is going into Aurora now?
> > > I think we should uplift the fix as soon as it's ready.
> > 
> > From what I can tell, this bug is a duplicate of bug 1010750, and should
> > have been fixed when that bug got fixed.
> > 
> > Marco, can you confirm this for me?
> 
> I haven't tested, but I don't believe it's fixed...  The WebappRT code
> path[1] likely needs similar fixes as were needed for Fennec in bug 1010750.
> 
> [1]:
> http://dxr.mozilla.org/mozilla-central/source/webapprt/RemoteDebugger.jsm#22

In that case, how can I test this code path?

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Eddy Bruel [:ejpbruel] from comment #6)
> (In reply to J. Ryan Stinnett [:jryans] from comment #5)
> > [1]:
> > http://dxr.mozilla.org/mozilla-central/source/webapprt/RemoteDebugger.jsm#22
> 
> In that case, how can I test this code path?

At the moment, it's a little tricky...  You'd need to install some web app from the marketplace into desktop Firefox, and then follow Myk's steps[1].

Eventually, these apps will be shown in the App Manager UI, but for now it's a bit tricky to do.

Marco or Myk may have better / more up to date advice.

[1]: https://hacks.mozilla.org/2014/03/better-integration-for-open-web-apps-on-android/comment-page-1/#comment-2159140

Comment 8

[Marco Castelluccio [:marco]]

(In reply to Eddy Bruel [:ejpbruel] from comment #6)
> (In reply to J. Ryan Stinnett [:jryans] from comment #5)
> > (In reply to Eddy Bruel [:ejpbruel] from comment #4)
> > > (In reply to Marco Castelluccio [:marco] from comment #3)
> > > > I guess this regression is going into Aurora now?
> > > > I think we should uplift the fix as soon as it's ready.
> > > 
> > > From what I can tell, this bug is a duplicate of bug 1010750, and should
> > > have been fixed when that bug got fixed.
> > > 
> > > Marco, can you confirm this for me?
> > 
> > I haven't tested, but I don't believe it's fixed...  The WebappRT code
> > path[1] likely needs similar fixes as were needed for Fennec in bug 1010750.
> > 
> > [1]:
> > http://dxr.mozilla.org/mozilla-central/source/webapprt/RemoteDebugger.jsm#22
> 
> In that case, how can I test this code path?

./mach webapprt-test-chrome

There's a browser_debugger.js test, but you can't run it individually (bug 996259).

Comment 9

[Eddy Bruel [:ejpbruel]]

Attachment: Proposed fix

This should take care of the problem.

Comment 10

[Marco Castelluccio [:marco]]

Comment on attachment 8438214 [details] [diff] [review]
Proposed fix

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

It would be great if you could also test manually to install an app
and launch it in debug mode (as explained in the link Ryan suggested
or at https://developer.mozilla.org/en-US/Marketplace/Options/Open_web_apps_for_desktop#Can_I_debug_my_apps_when_they_are_running_on_a_desktop.3F),
as the test doesn't cover all the code (even if I think it does cover
enough).

Comment 11

[Eddy Bruel [:ejpbruel]]

I've tested the fix locally as requested and it works for me.

https://hg.mozilla.org/integration/fx-team/rev/bd63a8720a1b

Does this need an aurora uplift as well?

Comment 12

[Marco Castelluccio [:marco]]

(In reply to Eddy Bruel [:ejpbruel] from comment #11)
> I've tested the fix locally as requested and it works for me.
> 
> https://hg.mozilla.org/integration/fx-team/rev/bd63a8720a1b
> 
> Does this need an aurora uplift as well?

It would be great if we could uplift to Aurora.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bd63a8720a1b

Comment 14

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8438214 [details] [diff] [review]
Proposed fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 859372
User impact if declined: Using the remote debugger to debug apps won't work
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): Marginal. The only changes involved are mechanical in nature.
String or IDL/UUID changes made by this patch:

Comment 15

[Lawrence Mandel [:lmandel] (use needinfo)]

From bug 859372 I take it this is a regression in 32 and that 31 and above are unaffected.

Comment 16

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8438214 [details] [diff] [review]
Proposed fix

Aurora approval granted.

Out of curiosity, why is this bug marked as leave open?

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/006afc4e1220

(In reply to Lawrence Mandel [:lmandel] from comment #16)
> Out of curiosity, why is this bug marked as leave open?

Going off the assumption that it was for the branch uplift. Eddy, note that bug resolution tracks landing on trunk, so leaving the bug open for branch uplifts only confuses things. That said, if I got that wrong, please reopen and whip me furiously for my insolence :P