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)
Tracking
(firefox35? fixed, firefox36 fixed)
RESOLVED
FIXED
Firefox 36
People
(Reporter: paul, Assigned: ochameau)
References
Details
Attachments
(3 files, 1 obsolete file)
1013 bytes,
patch
|
jryans
:
review+
|
Details | Diff | Splinter Review |
2.15 KB,
patch
|
ochameau
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
44 bytes,
text/x-github-pull-request
|
ochameau
:
review+
|
Details | Review |
Happens to me and jryans. No idea what's going on (no errors in the browser console).
Comment 1•10 years ago
|
||
This seems to happen with the latest nightly, without E10S.
Comment 2•10 years ago
|
||
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.
Updated•10 years ago
|
Assignee: nobody → pbrosset
Comment 3•10 years ago
|
||
From this conversation, it sounded like is was tied to a profile somehow: https://twitter.com/rich_goater/status/529339915008770048
Comment 4•10 years ago
|
||
(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. :/
Comment 6•10 years ago
|
||
The mentioned twitter report was Aurora 35 (so no dev edition stuff). Following up with him to try to get more info.
Comment 7•10 years ago
|
||
(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
Comment 8•10 years ago
|
||
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.
Reporter | ||
Comment 9•10 years ago
|
||
I can reproduce consistently. Windows. E10S is off.
Comment 10•10 years ago
|
||
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
Comment 11•10 years ago
|
||
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.
Comment 12•10 years ago
|
||
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).
Comment 13•10 years ago
|
||
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.
Comment 14•10 years ago
|
||
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.
Comment 15•10 years ago
|
||
(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.
Comment 16•10 years ago
|
||
(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.
Comment 17•10 years ago
|
||
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.
Comment 18•10 years ago
|
||
...and to be clear, can't repro at all in 35, so I suspect this is that e10s work??
Comment 19•10 years ago
|
||
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.
Comment 20•10 years ago
|
||
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)
Comment 21•10 years ago
|
||
(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]
Comment 22•10 years ago
|
||
still i see this problem in FF 34 beta 7. my e10s is enable. is e10s the reason?
Comment 23•10 years ago
|
||
(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.
Comment 24•10 years ago
|
||
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.
Comment 25•10 years ago
|
||
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.
Comment 26•10 years ago
|
||
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".
Comment 27•10 years ago
|
||
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.
Comment 28•10 years ago
|
||
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.
Comment 29•10 years ago
|
||
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)
Assignee | ||
Comment 30•10 years ago
|
||
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...
Assignee | ||
Comment 31•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0f06fe6877a2
Flags: needinfo?(poirot.alex)
Assignee | ||
Updated•10 years ago
|
Attachment #8521395 -
Flags: review?(jryans)
Comment 32•10 years ago
|
||
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.
Comment 33•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed,
leave-open
[Tracking Requested - why for this release]: Multiple reports that is affects Dev Edition
status-firefox35:
--- → affected
tracking-firefox35:
--- → ?
Comment 36•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7f1d95304dad
Keywords: checkin-needed
Comment 37•10 years ago
|
||
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.
Assignee | ||
Comment 38•10 years ago
|
||
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.
Assignee | ||
Updated•10 years ago
|
Attachment #8522260 -
Flags: review?(jryans)
Assignee | ||
Comment 39•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7bc113a7ea1f
Flags: needinfo?(dcamp)
Comment 40•10 years ago
|
||
(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+
Assignee | ||
Comment 43•10 years ago
|
||
With a test: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=dd3388bb4608
Attachment #8522260 -
Attachment is obsolete: true
Comment 44•10 years ago
|
||
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
Comment 45•10 years ago
|
||
(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.
Comment 48•10 years ago
|
||
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
Comment 49•10 years ago
|
||
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?
Comment 50•10 years ago
|
||
Same bug on ubuntu 14.04 x64 up to date.
Comment 51•10 years ago
|
||
Same bug on Mac OSX Yosemite, Firefox dev version 35.0a2 (2014-11-16)
Comment 52•10 years ago
|
||
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.
Comment 53•10 years ago
|
||
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.
Comment 54•10 years ago
|
||
(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!
Comment 55•10 years ago
|
||
(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
Comment 56•10 years ago
|
||
(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!
Comment 57•10 years ago
|
||
Confirmed for me as well. Reset -> Resync and it's working for me for now. OSX 10.9.5.
Comment 58•10 years ago
|
||
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)
Comment 60•10 years ago
|
||
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".
Assignee | ||
Comment 62•10 years ago
|
||
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+
Assignee | ||
Comment 63•10 years ago
|
||
Checking needded for the second attachment.
Keywords: leave-open → checkin-needed
Comment 64•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/1cdf743b10cb
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Comment 65•10 years ago
|
||
I'm not sure we found out when this started to happen, but the fix should probably be uplifted to at least Aurora, right?
Comment 66•10 years ago
|
||
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
Assignee | ||
Comment 67•10 years ago
|
||
(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...
Comment 69•10 years ago
|
||
(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)
Comment 70•10 years ago
|
||
(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.
Assignee | ||
Comment 71•10 years ago
|
||
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?
Assignee | ||
Comment 72•10 years ago
|
||
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.
Updated•10 years ago
|
Attachment #8522334 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•9 years ago
|
status-firefox36:
--- → fixed
Comment 75•9 years ago
|
||
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)
Assignee | ||
Comment 77•9 years ago
|
||
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)
Comment 78•9 years ago
|
||
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.
Assignee | ||
Comment 79•9 years ago
|
||
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.
Assignee | ||
Comment 80•9 years ago
|
||
Kamil, do you have any addon installed? (can you verify in about:addons?)
Comment 81•9 years ago
|
||
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?
Assignee | ||
Comment 82•9 years ago
|
||
Yes, can you try disabling ADB Helper and test again after restarting firefox?
Comment 83•9 years ago
|
||
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)
Assignee | ||
Comment 85•9 years ago
|
||
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?
Comment 86•9 years ago
|
||
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.
Updated•9 years ago
|
Flags: needinfo?(pbrosset)
Comment 87•9 years ago
|
||
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.
Comment 89•9 years ago
|
||
(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.
Assignee | ||
Comment 90•9 years ago
|
||
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.
Comment 93•9 years ago
|
||
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.
Assignee | ||
Comment 96•9 years ago
|
||
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)
Assignee | ||
Comment 99•9 years ago
|
||
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)
Comment 101•9 years ago
|
||
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.
Comment 103•9 years ago
|
||
(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
Depends on: 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)
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•