Closed Bug 1091706 Opened 10 years ago Closed 10 years ago

`Right click -> inspect` opens the inspector but doesn't select the element, only <body>.

Categories

(DevTools :: Inspector, defect)

x86_64
All
defect
Not set
normal

Tracking

(firefox35? fixed, firefox36 fixed)

RESOLVED FIXED
Firefox 36
Tracking Status
firefox35 ? fixed
firefox36 --- fixed

People

(Reporter: paul, Assigned: ochameau)

References

Details

Attachments

(3 files, 1 obsolete file)

Happens to me and jryans.

No idea what's going on (no errors in the browser console).
This seems to happen with the latest nightly, without E10S.
It also does not happen all the times according to jryans, which could point in the direction of a race condition.
The last bug that changed the right-click/inspect mechanism is bug 1039528, which was done for E10S support.
Assignee: nobody → pbrosset
From this conversation, it sounded like is was tied to a profile somehow: https://twitter.com/rich_goater/status/529339915008770048
(In reply to Brian Grinstead [:bgrins] from comment #3)
> From this conversation, it sounded like is was tied to a profile somehow:
> https://twitter.com/rich_goater/status/529339915008770048

It's pretty strange - can't repro personally but we have multiple reports. Are there factors in common eg OS? Fresh dev edition profile? etc.
I was on OS X in a profile where I had used e10s, but was not using it at the time.  Nightly build, no Dev. Edition.

However, I was unable to repro just now when I tried. :/
The mentioned twitter report was Aurora 35 (so no dev edition stuff).  Following up with him to try to get more info.
(In reply to J. Ryan Stinnett [:jryans] from comment #5)
> I was on OS X in a profile where I had used e10s, but was not using it at
> the time.  Nightly build, no Dev. Edition.
> 
> However, I was unable to repro just now when I tried. :/

FWIW he also mentioned that he had used e10s before: https://twitter.com/rich_goater/status/529406871648485376
I can reproduce this consistently on today's Dev Edition. A fresh profile fixes it. I on a mac and usually have e10s turned on.
I can reproduce consistently. Windows. E10S is off.
Just reproduced on devedition with E10S on.
This error might be related:

TypeError: this._form is undefined
Stack trace:
NodeFront<.attributes@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/inspector.js:836:20
NodeFront<.updateMutation@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/inspector.js:759:23
exports.WalkerFront<.getMutations</<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/inspector.js:2806:11
resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:40:40
then@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:20:43
resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:72:11
resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:40:11
then@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:20:43
resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:72:11
resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:40:11
then@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:20:43
resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:72:11
Front<.onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/protocol.js:1208:7
DebuggerClient.prototype.onPacket/<@resource://gre/modules/devtools/dbg-client.jsm:882:9
resolve@resource://gre/modules/devtools/deprecated-sync-thenables.js:40:40
then@resource://gre/modules/devtools/deprecated-sync-thenables.js:20:43
then@resource://gre/modules/devtools/deprecated-sync-thenables.js:58:9
DebuggerClient.prototype.onPacket@resource://gre/modules/devtools/dbg-client.jsm:869:1
LocalDebuggerTransport.prototype.send/<@resource://gre/modules/devtools/dbg-client.jsm -> resource://gre/modules/devtools/transport/transport.js:545:11
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:83:14
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:83:14
I got something weird while debugging nsContextMenu.js here: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js#518
|this.isRemote| returned false even with E10S activated, so of course the right-click/inspect element menu isn't going to work.
I'll investigate what could cause iRemote to be false.
Some post-investigation details:

- |this.isRemote| in nsContextMenu.js is false is |gContextMenuContentData| isn't set.
- |gContextMenuContentData| is only set when, with E10S, a right-click is made on content, which ends up sending a message (via the mm) to tabbrowser.xml which uses it to set |gContextMenuContentData|.
- Without E10S, there's no frame script that sends this event, so |gContextMenuContentData| is null, and therefore |this.isRemote| is false, and therefore we fall back to the old "inspect element" implementation which is something like |inspector.selection.setNode(this.target);| where |this.target| is a content DOM node.

This non mulit-process fallback used to work, and should still work. However, it now seems to cause the following error:

Full message: TypeError: source is not an object
Full stack: merge@resource://gre/modules/addons/XPIProvider.jsm -> file:///Users/pbrosset/Library/Application%20Support/Firefox/Profiles/bjnv8yqz.dev/extensions/fxos_1_4_simulator@mozilla.org/bootstrap.js -> resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/util/object.js:42:10
NodeFront<.form@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/inspector.js:739:18
types.addActorType/type<.read@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/protocol.js:282:7
exports.WalkerFront<.frontForRawNode@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/inspector.js:2866:22
Selection.prototype.setNode@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/framework/selection.js:136:1
CM_inspectNode/<@chrome://browser/content/nsContextMenu.js:523:9
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:868:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:747:7

Looking at protocol.js changes, I thing changeset 4cf592b066ef is the culprit.

So 2 solutions:
1 - either fix this and keep on using the non multi-process fallback when possible
2 - or always go via the frame script (my understanding is that the mm is transparent, whether you're on E10S or not, it's the same).

Solution 2 probably requires more changes because right now the frame script (browser/base/content/content.js) doesn't listen for contextmenu events if E10S isn't activated (see http://mxr.mozilla.org/mozilla-central/source/browser/base/content/content.js#90).
It gets weirder. I've also seen it fail with E10S (so, not using the fallback mechanism described above). And now, I *can't* get it to fail at all. I've tried with all my profiles, even new ones, with and without E10S, and it just works all the time now.
Patrick, I was in the twitter conversation mentioned by Brian above, and I can no longer replicate the issue either. I thought it was related to disabling the Firefox OS simulators, but with both 1.3 and 2.2 enabled now, the problem no longer arises. I would say that the inspector does seem to linger on the body tag for a moment before moving to the inspected tag, but at least it is working.
(In reply to richard.goater27 from comment #14)
> Patrick, I was in the twitter conversation mentioned by Brian above, and I
> can no longer replicate the issue either. I thought it was related to
> disabling the Firefox OS simulators, but with both 1.3 and 2.2 enabled now,
> the problem no longer arises. I would say that the inspector does seem to
> linger on the body tag for a moment before moving to the inspected tag, but
> at least it is working.
Did in update firefox since? Which version are you on now?
I'm trying to find out if it could have been fixed by another bug that landed in the meantime.
(In reply to richard.goater27 from comment #14)
> I would say that the inspector does seem to
> linger on the body tag for a moment before moving to the inspected tag, but
> at least it is working.
The delay you're seeing is the time it takes for the inspector to get initialized and for the DOM tree data to be passed to the client-side, I don't think it's related to the issue we're seeing here.
I can still repro in my nightly. I did have the simulators disabled, but I re-enabled them ( 1.3, 1.4 and 2.0 ) and still see the problem.

Elusive, beast, this.
...and to be clear, can't repro at all in 35, so I suspect this is that e10s work??
Still can't reproduce ... I've been downloading several nightlies from the last few days, tested with/without E10S, with existing/new profiles ... I don't know how I managed to get it failing yesterday for some time and then not ever again.
Looking at the stacktrace in comment 12 (which happened in non-E10S mode), I can say the following:

- NodeFront.form [1] tries to execute |this._form = object.merge(form);| with |form| being a string, which obviously fails.
- |form| can only be a string if its the actorID, but in that case, |detail| is "actorid" and so the method early returns before trying to merge the object.
- The method is called as a result of calling |frontForRawNode| [2], and more precisely when converting the NodeActor to a NodeFront here [3].

Dave: since I can't repro, I can't debug. Do you have any idea how this line [2] could end up causing the error?

[1] http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/inspector.js#731
[2] http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/inspector.js#2853
[3] http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/inspector.js#2866
Flags: needinfo?(dcamp)
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #20)
> Dave: since I can't repro, I can't debug. Do you have any idea how this line
> [2] could end up causing the error?
Sorry, I meant [3]
still i see this problem in FF 34 beta 7. my e10s is enable. is e10s the reason?
(In reply to Saad Shamsaei from comment #22)
> still i see this problem in FF 34 beta 7. my e10s is enable. is e10s the
> reason?
Well we've had reports of people facing this bug with both e10s enabled and disabled. Knowing how this feature is implemented, it means we essentially have 2 bugs, because the code path taken with e10s is very different from the one taken without.
You may want to try without e10s, to see if that solves the issue for you though.
this option was true on my FF:
'browser.tabs.remote.autostart'

i set false it and restart. but problem not has not fixed.

this option was false and also now is false:
'browser.tabs.remote'

maybe this problem is not depended to e10s.
I updated my local fx-team build and reproduced the problem with my usual dev profile. I have no idea why it would manifest itself now knowing that it didn't last week on fx-team with the same profile.

- Upon starting the build, the default tab that opened doesn't have the underlined title, and executing |content| from the scratchpad (in browser environment) does return the window object (not a CPOW), which means this window is not e10s (although I thought e10s was on by default now).
- When debugging the right-click issue, I see that the node actor it tries to access has an ID that starts with "conn0.child1/" which is usually the prefix we use for e10s connections.
- In WalkerFront.frontForRawNode, we do detect that the connection is local, which is in line with my assumption that this is a non e10s window, and the connection's prefix is "conn0." (obtained via |this.conn._transport._serverConnection._prefix|), which is also correct assuming this is a local (non-e10s) tab.

So how can we have both a connection with prefix "conn0.child1/" and one with prefix "conn0.".
Obviously, the code fails with a "no such actor for ID...." error.
Even weirder is that if I hover over nodes in the markup-view to highlight them, I do see the highlighter coming up in the page, but I also see errors like:

DBG-SERVER: Received packet 471: {
  "from": "conn0.child1/highlighter24",
  "error": "noSuchActor",
  "message": "No such actor for ID: conn0.child1/highlighter24"
}

How could a protocol request fail and succeed at the same time? 
Looking at protocol packets in the logs, I only see "conn0.child1/highlighter24" types of packets, nothing targeted at "conn0.highlighterxxx".
Just figured out that the about:sessionrestore page might be part of the problem. Even with e10s on, when firefox starts with this page, it apparently displays it with a non-e10s window, but still the devtools connection has the "conn0.child1/" prefix.
On the about:sessionrestore page, the |DebuggerServerConnection.onPacket| method actually gets called twice.
One of the instance it is called on has the expected |_forwardingPrefixes| map entry, the other one doesn't which ends up generating an error.
So, on this page at least, it looks like we have 2 connections instead of one, and only one works.
Alex, I think you wrote (at least part of) this code, what is your take on this:
on about:sessionrestore, the front actors have IDs prefixed with conn0.child1/, but the server connection has prefix conn0. It also has a _forwardingPrefixes set.
However about:sessionrestore is opened in a non-e10s window.
Flags: needinfo?(poirot.alex)
We were juste considering <browser remote="false"> as being remote.
Actually, any browser whose remote attribute was non-falsy js value...

This should fix the weird behavior you have seen,
but doesn't fix protocol.js issue.

With that patch, I've been able to reproduce this issue consistantly
on about:sessionrestore, as described in comment 20.

Reverting bug 1072080 didn't fixed the issue for me,
even if this patch seems related to this exception...
Attachment #8521395 - Flags: review?(jryans)
Latest Dev Edition on OSX 10.9. Started Dev Edition with a new profile and can still reproduce. I've yet to get "inspect element" from the context menu to work at all in this version. Have never used E10S with this profile.
More info: It does NOT happen if I start latest Dev Edition in a new profile, so it must be due to the fact that my current dev edition profile was created when Dev Edition was a nightly before release.
Comment on attachment 8521395 [details] [diff] [review]
Properly create TabActor on non-e10s tabs - v1

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

Good catch!
Attachment #8521395 - Flags: review?(jryans) → review+
[Tracking Requested - why for this release]: Multiple reports that is affects Dev Edition
Thanks Alex for the patch, this helps.

I can reproduce the problem in non-e10s windows consistently now, and I figured that the bug comes from this line http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/inspector.js#2866
This read/write pair mechanism is done to create a NodeFront, but the nodeActor passed through it is special: |nodeActor instance protocol.Actor| evaluates to false, and since protocol.js relies on it to know if the object being passed is an actor, this makes the rest of the code path to fail.

I have a feeling this is due to xray wrapping on the nodeActor object, but waiving the wrapper doesn't seem to make this condition true either.
OMG.

We were spawning one new loader instance (and a full set of new module instances)
every time we used Cu.import(Loader.jsm).devtools.require.
Here I've seen one new instance per webide addon (adb helper and fxos-adapters),
but I think it will be the same with the various code in m-c that does that :/

This is frightening that anything works,
we end up with a mix of modules with duplicated and eventually different states.
It can fix various inexplicable bugs.
Attachment #8522260 - Flags: review?(jryans)
(In reply to Alexandre Poirot [:ochameau] from comment #38)
> Created attachment 8522260 [details] [diff] [review]
> Ensure spawning only one devtools loader instances - v1
> 
> OMG.
> 
> We were spawning one new loader instance (and a full set of new module
> instances)
> every time we used Cu.import(Loader.jsm).devtools.require.
> Here I've seen one new instance per webide addon (adb helper and
> fxos-adapters),
> but I think it will be the same with the various code in m-c that does that
> :/
> 
> This is frightening that anything works,
> we end up with a mix of modules with duplicated and eventually different
> states.
> It can fix various inexplicable bugs.
This patch seems to do the trick.
I can't reproduce the error in non-e10s windows anymore (and e10s windows worked fined for me already).
I had another patch that fixed the issue by going around it basically, but if this fixes the root cause, that's better.
Comment on attachment 8522260 [details] [diff] [review]
Ensure spawning only one devtools loader instances - v1

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

This is a better catch than the last one! :)

For bonus points, a test to check this behavior would be great.  Should be easy to write I think...
Attachment #8522260 - Flags: review?(jryans) → review+
Assigning this to its rightful owner :)
Assignee: pbrosset → poirot.alex
Not sure if it helps, but with todays build of the Developers Edition on Mac "35.0a2 (2014-11-13)" this bug is still there when using the default profile created with the original install. When using a clean profile it works. There is also an error in stdout that might be of interest as it seems related (only shows when inspector is not working, not with working inspector on the clean profile):

A coding exception was thrown in a Promise resolution callback.
See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise

Full message: TypeError: source is not an object
Full stack: merge@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/util/object.js:43:10
NodeFront<.form@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/inspector.js:727:18
types.addActorType/type<.read@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/protocol.js:282:7
exports.WalkerFront<.frontForRawNode@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/inspector.js:2854:22
Selection.prototype.setNode@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/framework/selection.js:136:1
CM_inspectNode/<@chrome://browser/content/nsContextMenu.js:512:9
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:865:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:744:7
(In reply to ralfhein from comment #44)
> Not sure if it helps, but with todays build of the Developers Edition on Mac
> "35.0a2 (2014-11-13)" this bug is still there when using the default profile
> created with the original install. When using a clean profile it works.
> There is also an error in stdout that might be of interest as it seems
> related (only shows when inspector is not working, not with working
> inspector on the clean profile):
> 
> A coding exception was thrown in a Promise resolution callback.
> See
> https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/
> Promise
> 
> Full message: TypeError: source is not an object
> Full stack: merge@resource://gre/modules/commonjs/toolkit/loader.js ->
> resource://gre/modules/commonjs/sdk/util/object.js:43:10
> NodeFront<.form@resource://gre/modules/commonjs/toolkit/loader.js ->
> resource://gre/modules/devtools/server/actors/inspector.js:727:18
> types.addActorType/type<.read@resource://gre/modules/commonjs/toolkit/loader.
> js -> resource://gre/modules/devtools/server/protocol.js:282:7
> exports.WalkerFront<.frontForRawNode@resource://gre/modules/commonjs/toolkit/
> loader.js ->
> resource://gre/modules/devtools/server/actors/inspector.js:2854:22
> Selection.prototype.setNode@resource://gre/modules/commonjs/toolkit/loader.
> js -> resource:///modules/devtools/framework/selection.js:136:1
> CM_inspectNode/<@chrome://browser/content/nsContextMenu.js:512:9
> Handler.prototype.process@resource://gre/modules/Promise.jsm ->
> resource://gre/modules/Promise-backend.js:865:23
> this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm ->
> resource://gre/modules/Promise-backend.js:744:7
Yes that's the same error we've been seeing. Alex's patch will fix this.
I have been able to reproduce this bug on all versions of Developer Edition so far including the one that has just installed for me, I am using Win7x64
I am able to reproduce this bug on Windows 7x64 with latest updates installed,  However it does not occur with MAC OS X (Yosemite), anyone test with linux?
Same bug on ubuntu 14.04 x64 up to date.
Same bug on Mac OSX Yosemite, Firefox dev version 35.0a2 (2014-11-16)
35.0a2 (2014-11-15) on Mac OSX Yosemite seems to be working (right click - inspect element) for me.  Cant get it to work on my windows 7 machine.
This was not working for me every time. Then I signed out of sync, reset Firefox via Help (?) > Troubleshooting Info, and re-synced and it's working now. I'll post again if it stops.
(In reply to Jon from comment #53)
> This was not working for me every time. Then I signed out of sync, reset
> Firefox via Help (?) > Troubleshooting Info, and re-synced and it's working
> now. I'll post again if it stops.

That fixed it for me, thanks!
(In reply to Hiram Hibbard from comment #54)
> (In reply to Jon from comment #53)
> > This was not working for me every time. Then I signed out of sync, reset
> > Firefox via Help (?) > Troubleshooting Info, and re-synced and it's working
> > now. I'll post again if it stops.
> 
> That fixed it for me, thanks!

This fixed it for me too on Ubuntu 14.04
(In reply to Jon from comment #53)
> This was not working for me every time. Then I signed out of sync, reset
> Firefox via Help (?) > Troubleshooting Info, and re-synced and it's working
> now. I'll post again if it stops.

Works for me too, thanks a lot!
Confirmed for me as well. Reset -> Resync and it's working for me for now. OSX 10.9.5.
Confirmed Reset works on win7x64: If you need help follow steps mentioned here: http://stackoverflow.com/questions/26913853/odd-behavior-in-firefox-developer-edition-the-inspect-element-context-menu-op/26949568#26949568

Can say that I at first tried uninstalling firefox and then reinstalling but that did not fix my problem.  it was not until I did the actual refresh that the problem went away.
Alex, is your second patch here ready to land?  Try seems green.
Flags: needinfo?(poirot.alex)
Will this second patch fix things for people without them needing to do the full reset / resync?  We should land and request uplift ASAP if so.
Status: NEW → ASSIGNED
(In reply to Brian Grinstead [:bgrins] from comment #60)
> Will this second patch fix things for people without them needing to do the
> full reset / resync?  We should land and request uplift ASAP if so.

The second patch fixes what is essentially a race condition in our loader that can cause duplicate, separate loaders to exist given a specific pattern of calls.

My guess is that reset / resync is somehow changing the order in which the calls happen, perhaps due the add-ons being removing and retrieved again, which appears to work around the issue, at least for the moment.

So, tentatively I would say "yes".
Comment on attachment 8522334 [details] [diff] [review]
Ensure spawning only one devtools loader instances - v2

Yes, it's ready.
Flags: needinfo?(poirot.alex)
Attachment #8522334 - Flags: review+
Checking needded for the second attachment.
I'm not sure we found out when this started to happen, but the fix should probably be uplifted to at least Aurora, right?
https://hg.mozilla.org/mozilla-central/rev/1cdf743b10cb
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 36
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #65)
> I'm not sure we found out when this started to happen, but the fix should
> probably be uplifted to at least Aurora, right?

Is dev edition based on FF36? Because we got many report from dev-edition being broken.
The issue itself seems to be here since many many releases but it may only be trigerred with some addons introduced more recently...
(In reply to Alexandre Poirot [:ochameau] from comment #67)
> (In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #65)
> > I'm not sure we found out when this started to happen, but the fix should
> > probably be uplifted to at least Aurora, right?
> 
> Is dev edition based on FF36? Because we got many report from dev-edition
> being broken.

devedition === aurora, so right now it's FF35. if at all possible, this should be uplifted.
Flags: needinfo?(poirot.alex)
(In reply to :Gijs Kruitbosch from comment #69)

> devedition === aurora, so right now it's FF35. if at all possible, this
> should be uplifted.

+1.
Comment on attachment 8522334 [details] [diff] [review]
Ensure spawning only one devtools loader instances - v2

Approval Request Comment
[Feature/regressing bug #]:
  Very old regression, but seems to only happen when some specific addons get installed, like webide helper addons or sync.
[User impact if declined]:
  Broken feature in inspector.
[Describe test coverage new/current, TBPL]:
  Specific test introduced to cover just this particular regression.
  Manually applied the patch on aurora, no conflict, fixes the issue also on aurora.
[Risks and why]:
[String/UUID change made/needed]:
  None
Flags: needinfo?(poirot.alex)
Attachment #8522334 - Flags: approval-mozilla-aurora?
Comment on attachment 8521395 [details] [diff] [review]
Properly create TabActor on non-e10s tabs - v1

This patch is not necessary on aurora as the related regression is recent and only exists on master.
Attachment #8522334 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
What about Firefox 34. Will it be fixed in this version? (I have Firefox Beta from mozillateam PPA)
Is this believed to be an issue in 34?  Or would it be a different bug?
Flags: needinfo?(poirot.alex)
Flags: needinfo?(pbrosset)
I can't explain why, but it doesn't seem to happen on 34!?

The code I modified seems to be here for quite a while:
  http://mxr.mozilla.org/mozilla-beta/source/toolkit/devtools/Loader.jsm#302
Flags: needinfo?(poirot.alex)
I say, what I see. I have Firefox 34 on Ubuntu 14.10 and when I click Inspect element, it selects only <body>, so I think it is same bug.
At least, I wasn't able to reproduce my testcase (that reproduced on master and DevEdition consistantly) against about:sessionrestore when having adb helper addon installed.
Kamil, do you have any addon installed? (can you verify in about:addons?)
Yes, i have some addons:
- ADB Helper
- Adblock Plus
- ColorZilla
- Gmail Notifier
- GNotifier
- Hide Plugin Notifications
- LastPass
- Ubuntu Firefox Modifications

Do you think, it can be caused by one of them?
Yes, can you try disabling ADB Helper and test again after restarting firefox?
Wow. You were right.
But disabling it isn't really a solution, because I need ADB Helper to debug my Firefox OS device. 
Have you any other idea what I can do?
Should we uplift to beta then?  Though could be tough to get approval at this point...
Flags: needinfo?(poirot.alex)
It really looks like you are facing the same issue I fixed, but I'm not able to reproduce it locally.
Can you describe your testcase? Is it reproducible everytime?
Yes, it's reproducible every time.
I found this today, when I was testing project, on which I am working. 
I think it started when I upgraded from stable to beta, but I'm not sure, because I didn't use Inspector until today.

I do not have any testcase.
I just tried inspecting elements on facebook, here on bugzilla and on my project, it's always same.
Flags: needinfo?(pbrosset)
I replicated this on Firefox 34. To my astonishment, ADB Helper was installed and disabling it fixed the issue. I don't know how ADB Installer got installed; perhaps from running Aurora/Developer Edition.
(In reply to Greg Edwards from comment #87)
> I replicated this on Firefox 34. To my astonishment, ADB Helper was
> installed and disabling it fixed the issue. I don't know how ADB Installer
> got installed; perhaps from running Aurora/Developer Edition.

Yes, Dev Edition auto-installs the add-on.
(In reply to J. Ryan Stinnett [:jryans] from comment #88)

> Yes, Dev Edition auto-installs the add-on.

I thought it was the case that opening WebIDE once installed the add-on, regardless of the version / edition of Firefox.
Unfortunately it is too late for 34, but may be we can introduce something in adb helper?
I don't know how, but we would need to use require() differently, may be find a way to use require() without Loader.jsm...
Flags: needinfo?(poirot.alex)
(In reply to Jeff Griffiths (:canuckistani) from comment #89)
> (In reply to J. Ryan Stinnett [:jryans] from comment #88)
> 
> > Yes, Dev Edition auto-installs the add-on.
> 
> I thought it was the case that opening WebIDE once installed the add-on,
> regardless of the version / edition of Firefox.

Ah, sorry, I was thinking of Valence, which is only auto-installed on Dev Edition after WebIDE open.

Yes, ADB Helper is auto-installed after WebIDE open on all channels.
(In reply to Alexandre Poirot [:ochameau] from comment #90)
> Unfortunately it is too late for 34, but may be we can introduce something
> in adb helper?
> I don't know how, but we would need to use require() differently, may be
> find a way to use require() without Loader.jsm...

That sounds complex to me...  It may be too late to do anything reasonable here for 34.
What about pushing an update to the ADB Helper plugin that disables it (or whatever's necessary to fix the problem) if it detects Firefox 34 is running? Most users should have auto update enabled for addons.
(In reply to Greg Edwards from comment #93)
> What about pushing an update to the ADB Helper plugin that disables it (or
> whatever's necessary to fix the problem) if it detects Firefox 34 is
> running? Most users should have auto update enabled for addons.

We could land some kind of fix in ADB Helper, but it could be complex to devise.

It doesn't seem appropriate to just disable ADB Helper in 34 to make the inspector work again, as you then have no way to use ADB Helper there, and it's not really ADB Helper's fault (it just triggers a problem created by core DevTools code).

It still may not help if people have other add-ons that use the DevTools APIs, as they could also trigger similar issues.
I tried to provide a fix from adb helper but I'm not able to reproduce the issue on Fx34 linux/windows, nor Fx dev edition. Ryan, were you able to reproduce it??
(In reply to Alexandre Poirot [:ochameau] from comment #96)
> I tried to provide a fix from adb helper but I'm not able to reproduce the
> issue on Fx34 linux/windows, nor Fx dev edition. Ryan, were you able to
> reproduce it??

Just tried.  I am able to reproduce on Mac 34.0.5 with ADB Helper 0.7.1.

Let me see if there is a quick fix...
Patching require from the add-on worked for me.  Will want to get verification before releasing this from someone else as well.
Attachment #8537884 - Flags: review?(poirot.alex)
Comment on attachment 8537884 [details] [review]
Patch devtools.require on Firefox 34

Ship it.
Attachment #8537884 - Flags: review?(poirot.alex) → review+
(In reply to Greg Edwards from comment #93)
> What about pushing an update to the ADB Helper plugin that disables it (or
> whatever's necessary to fix the problem) if it detects Firefox 34 is
> running? Most users should have auto update enabled for addons.

Greg, can you test the 0.7.2pre build[1] on Fx 34?  If this fixes the issue for you as well, I'll release an update.

[1]: http://people.mozilla.org/~rstinnett/
Flags: needinfo?(edwardsgreg)
Yes, this version solves the issue for me too.
Flags: needinfo?(edwardsgreg)
(In reply to Greg Edwards from comment #101)
> Yes, this version solves the issue for me too.

Great!  I've released 0.7.2, so hopefully we've now covered all versions of Firefox.
(In reply to J. Ryan Stinnett [:jryans] from comment #102)
> (In reply to Greg Edwards from comment #101)
> > Yes, this version solves the issue for me too.
> 
> Great!  I've released 0.7.2, so hopefully we've now covered all versions of
> Firefox.

The plugin update is causing what looks like a pretty bad regression: Bug 1113539
I've reverted the ADB Helper change in version 0.7.3.  As noted, the attempt to patch things up in 0.7.2 caused far more serious issues.

This does mean that the broken inspector behavior will return in Firefox 34 for profiles with ADB Helper.  If this affects you, your best options are to either:

* remove ADB Helper, or
* move to a more recent channel (Beta or newer)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.