Irakli Gozalishvili [:irakli] [:gozala] [@gozala] Assignee
	Description • 10 years ago

If you register a tool that adds type=content iframe into toolbox docking it to the side breaks. Error is caused by `swapFrameLoader` presumable because swapping with content iframes in chrome iframes isn't implemented on the platform.

	Irakli Gozalishvili [:irakli] [:gozala] [@gozala] Assignee
	Comment 1 • 10 years ago

Paul I think you said you could look into this, could you please do that ?

	Paul Rouget [:paul]
	Comment 2 • 10 years ago

Boris, the devtools iframe holds multiple chrome docshells. We use swapFrameLoader to undock the devtools into other iframes.

Adding a content iframe breaks swapFrameLoader. Is that expected? If so, how can we fix that?

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 10 years ago

swapFrameLoader was designed to implement moving tabs between windows.  Everything else is gravy.

In particular, it ensures that the two types of docshells it's swapping are same-type.  This is very important at least because different types of docshells require different treeowners and swapping simply swaps treeowners.

So yes, the behavior you're seeing is expected.  The way to fix it is to not try to swap different types of iframes: it's not supported, and we don't particularly want to try supporting it; it's likely to be very complicated and dangerous in security terms, for minimal gain.

A tool that really wants to have all its stuff in a type="content" iframe can put that <irame> inside a chrome one, swap the outer thing and put all its stuff in the inner thing.  It's a bit ugly, but it should work...

	Irakli Gozalishvili [:irakli] [:gozala] [@gozala] Assignee
	Comment 6 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #3)
> swapFrameLoader was designed to implement moving tabs between windows. 
> Everything else is gravy.
>

Would you suggest some other way to do implement devtools docking feature that aligns devtools toolbox either to the side or to the bottom ?

> 
> In particular, it ensures that the two types of docshells it's swapping are
> same-type.  This is very important at least because different types of
> docshells require different treeowners and swapping simply swaps treeowners.
> 
> So yes, the behavior you're seeing is expected.  The way to fix it is to not
> try to swap different types of iframes: it's not supported, and we don't
> particularly want to try supporting it; it's likely to be very complicated
> and dangerous in security terms, for minimal gain.
>

I think you may have misunderstood, docshells being swapped are both of chrome type, it's just if chrome document contains a sub-iframe of content type that seems to break swapFrameLoader as well.


> 
> A tool that really wants to have all its stuff in a type="content" iframe
> can put that <irame> inside a chrome one, swap the outer thing and put all
> its stuff in the inner thing.  It's a bit ugly, but it should work...

I don't really understand what do you mean by put all it's stuff in the inner thing, but I think what you are describing is more or less what we have. Developer toolbox is defined in xul document that is loaded into chrome type iframe. Then each tab in the developer toolbox is loaded in it's own iframe, built-in ones are of chrome type, while ones defined by extensions are of content type. When toolbox is docked to the side outer iframe is swapped with another chrome type iframe, but following exceptions is raised:

`Exception... "Method not implemented"  nsresult: "0x80004001 (NS_ERROR_NOT_IMPLEMENTED)`

	Boris Zbarsky [:bzbarsky]
	Comment 7 • 10 years ago

> I think you may have misunderstood, docshells being swapped are both of chrome type

That was entirely unclear from comment 0 and comment 2, you might note.

But you're right that what you're actually doing is what I suggested.  And you're right that it doesn't work, because of this bit in nsFrameLoader::SwapWithOtherLoader:

1107   // One more twist here.  Setting up the right treeowners in a heterogeneous
1108   // tree is a bit of a pain.  So make sure that if ourType is not
1109   // nsIDocShellTreeItem::typeContent then all of our descendants are the same
1110   // type as us.
1111   if (ourType != nsIDocShellTreeItem::typeContent &&
1112       (!AllDescendantsOfType(ourDocshell, ourType) ||
1113        !AllDescendantsOfType(otherDocshell, otherType))) {
1114     return NS_ERROR_NOT_IMPLEMENTED;

Fixing that is ... I'm not sure.  I'd have to study the treeowner bits pretty carefully, because it's not like they're documented at all in terms of the invariants they rely on.  :(  We'd at the least need to pass both the chrome and content tree owners to SetTreeOwnerAndChromeEventHandlerOnDocshellTree and set the right one based on the item type.  And replace the AllDescendantsOfType bits with getting the content tree owner, if any.  That won't do the right AddTreeItemToTreeOwner bits for the content tree owner, though, and I suspect we do need to do that.

Plus need to sort out what the story is with the chrome event handler, I guess, but I expect this to be simpler.

Just to check, what is the use case for tools using type="content" iframes?  We can make this work with enough effort, but the effort will be pretty nontrivial.  :(

	Boris Zbarsky [:bzbarsky]
	Comment 8 • 10 years ago

Also, I'm starting to really wish I'd pushed back when people initially requested being able to swap chrome-type iframes at all... ;)

	Boris Zbarsky [:bzbarsky]
	Comment 9 • 10 years ago

To answer your other question.... If you're moving the devtools toolbox between windows, I have no other suggestions.  If you're moving it within a single window, feels like we might be able to find something else, maybe.

	Irakli Gozalishvili [:irakli] [:gozala] [@gozala] Assignee
	Comment 10 • 10 years ago

(In reply to Please do not ask for reviews for a bit [:bz] from comment #7)
> 
> Just to check, what is the use case for tools using type="content" iframes? 
> We can make this work with enough effort, but the effort will be pretty
> nontrivial.  :(


Add-on defined tools get content type iframes, it's also possible that some of the built-in tools will migrate to content iframes in the future.


Interesting fact, that is probably worth mentioning, is that if we make that content iframe remote (by setting remote attribute to true) everything works fine on OSX, unfortunately it caused some issues on Windows. If use of remote content iframes is supposed to work, we can try to go that route and find a fix for windows.

	Irakli Gozalishvili [:irakli] [:gozala] [@gozala] Assignee
	Comment 11 • 10 years ago

(In reply to Please do not ask for reviews for a bit [:bz] from comment #9)
> To answer your other question.... If you're moving the devtools toolbox
> between windows, I have no other suggestions.  If you're moving it within a
> single window, feels like we might be able to find something else, maybe.

Initially I was under impression that we needed this to move toolbox from bottom to the side, but it seems that we can also detach toolbox into separate window, so I'm afraid we won't be able to get away without swapping frame loaders.

	Boris Zbarsky [:bzbarsky]
	Comment 12 • 10 years ago

> Add-on defined tools get content type iframes

OK.  Can we change that for the moment until we get this sorted out?  ;)

> is that if we make that content iframe remote (by setting remote attribute to true)
> everything works fine on OSX,

Well, at least it doesn't throw immediately since we can't walk into the subframe and find it.  Whether things are correctly hooked up to the treeowners is ... I have no idea.  Depends on how that's handled for remote iframes, exactly.

> so I'm afraid we won't be able to get away without swapping frame loaders.

Then someone probably needs to sit down and update tree owners correctly (whatever that means; figuring that out is part of the task) on the entire frame subtree here.

	Jeff Griffiths (:canuckistani) (:⚡︎)
	Comment 13 • 10 years ago

cc'ing Joe, this is pretty important for the 37 cycle IMO.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 16 • 10 years ago

Comment on attachment 8546311 [details] [diff] [review]
Add forcemessagemanager attribute to force message manager on iframe

This will make message manager hierarchy a bit unusual in some case, but
not too bad. We can have similar hierarchy already now if
one has both chrome and content browsers in the same level in document 
hierarchy and then chrome browser contains from content browsers.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 17 • 10 years ago

Pushed to try while Irakli's push access gets restored: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c46ab8497d11

	Erik Vold [:erikvold] (please needinfo? me)
	Comment 18 • 10 years ago

Comment on attachment 8546315 [details]
Fix for on the SDK side

I see some issues that don't make sense to me which I commented on in the pull request:

1. It seems like a use of an arrow function would cause `this` usage within it to be an unexpected value.
2. I don't understand where `addEventListener` is defined.
3. I see failures in the travis build (see below)
4. This needs more tests I think, I mentioned that I don't see one to make sure remote content is not used in a dev/panel.

JPM info console.info: addon-sdk: executing './test/test-dev-panel.test Panel API'

JPM info console.info: addon-sdk: pass: panel is defined

JPM info console.info: addon-sdk: pass: tool is defined

JPM info console.log: addon-sdk: [JavaScript Warning: "SyntaxError: in strict mode code, functions may be declared only at top level or immediately within another function" {file: "resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/touch-events.js" line: 222 column: 17 source: "        function clone(obj) {
"}]

JPM error JavaScript error: resource://extensions.modules.addon-sdk.commonjs.path./toolkit/loader.js -> resource://extensions.modules.addon-sdk.commonjs.path./dev/panel.js, line 205: TypeError: messageManager is null

JPM info console.log: addon-sdk: [JavaScript Error: "TypeError: messageManager is null" {file: "resource://extensions.modules.addon-sdk.commonjs.path./toolkit/loader.js -> resource://extensions.modules.addon-sdk.commonjs.path./dev/panel.js" line: 205}]

JPM info console.error: addon-sdk: 

JPM info   fail:
  Timed out (after: tool is defined)

JPM info console.info: addon-sdk: executing './test/test-dev-panel.test Panel communication'

JPM error JavaScript error: resource://extensions.modules.addon-sdk.commonjs.path./toolkit/loader.js -> resource://extensions.modules.addon-sdk.commonjs.path./dev/panel.js, line 205: TypeError: messageManager is null

JPM info console.log: addon-sdk: [JavaScript Error: "TypeError: messageManager is null" {file: "resource://extensions.modules.addon-sdk.commonjs.path./toolkit/loader.js -> resource://extensions.modules.addon-sdk.commonjs.path./dev/panel.js" line: 205}]

JPM info console.error: addon-sdk: 

JPM info   fail:

JPM info   Timed out (after: START)

JPM info console.info: addon-sdk: executing './test/test-dev-panel.test communication with debuggee'

JPM error JavaScript error: resource://extensions.modules.addon-sdk.commonjs.path./toolkit/loader.js -> resource://extensions.modules.addon-sdk.commonjs.path./dev/panel.js, line 205: TypeError: messageManager is null

JPM info console.log: addon-sdk: [JavaScript Error: "TypeError: messageManager is null" {file: "resource://extensions.modules.addon-sdk.commonjs.path./toolkit/loader.js -> resource://extensions.modules.addon-sdk.commonjs.path./dev/panel.js" line: 205}]

JPM info console.error: addon-sdk: 

JPM info   fail:

JPM info   Timed out (after: START)

JPM info console.info: addon-sdk: executing './test/test-dev-panel.test createView panel'

JPM error JavaScript error: resource://extensions.modules.addon-sdk.commonjs.path./toolkit/loader.js -> resource://extensions.modules.addon-sdk.commonjs.path./sdk/context-menu/core.js, line 79: TypeError: target.messageManager is undefined

JPM info console.log: addon-sdk: [JavaScript Error: "TypeError: target.messageManager is undefined" {file: "resource://extensions.modules.addon-sdk.commonjs.path./toolkit/loader.js -> resource://extensions.modules.addon-sdk.commonjs.path./sdk/context-menu/core.js" line: 79}]

JPM info console.info: addon-sdk: pass: panel passed to createView is one instantiated

JPM info console.info: addon-sdk: pass: createView has created an iframe

JPM info console.info: addon-sdk: pass: is expected iframe

JPM error JavaScript error: resource://extensions.modules.addon-sdk.commonjs.path./dev/frame-script.js, line 76: TypeError: this is undefined

JPM info console.log: addon-sdk: [JavaScript Error: "TypeError: this is undefined" {file: "resource://extensions.modules.addon-sdk.commonjs.path./dev/frame-script.js" line: 76}]

JPM info console.info: addon-sdk: executing './test/test-dev-panel.test viewFor panel'

JPM error JavaScript error: resource://extensions.modules.addon-sdk.commonjs.path./toolkit/loader.js -> resource://extensions.modules.addon-sdk.commonjs.path./dev/panel.js, line 205: TypeError: messageManager is null

JPM info console.log: addon-sdk: [JavaScript Error: "TypeError: messageManager is null" {file: "resource://extensions.modules.addon-sdk.commonjs.path./toolkit/loader.js -> resource://extensions.modules.addon-sdk.commonjs.path./dev/panel.js" line: 205}]

JPM error JavaScript error: resource://gre/modules/SimpleServiceDiscovery.jsm, line 121: NS_ERROR_XPC_GS_RETURNED_FAILURE: Component returned failure code: 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService]

JPM info console.log: addon-sdk: [JavaScript Error: "NS_ERROR_XPC_GS_RETURNED_FAILURE: Component returned failure code: 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService]" {file: "resource://gre/modules/SimpleServiceDiscovery.jsm" line: 121}]

JPM info console.error: addon-sdk: 

JPM info   fail:
  Timed out (after: START)

JPM info console.info: addon-sdk: executing './test/test-diffpatcher.test diffpatcher.test diff.test add update'

	Irakli Gozalishvili [:irakli] [:gozala] [@gozala] Assignee
	Comment 19 • 10 years ago

(In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #18)
> Comment on attachment 8546315 [details]
> Fix for on the SDK side
> 
> I see some issues that don't make sense to me which I commented on in the
> pull request:
> 
> 1. It seems like a use of an arrow function would cause `this` usage within
> it to be an unexpected value.

I've addressed that, I just changed few things before submitting pull & introduced this.

> 2. I don't understand where `addEventListener` is defined.

It's defined in the environment into which framescripts execute, see link below for details
https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/Frame_script_environment

> 3. I see failures in the travis build (see below)

That is expected as this pull depends on C++ change in the other patch.

> 4. This needs more tests I think, I mentioned that I don't see one to make
> sure remote content is not used in a dev/panel.
> 

I've added a test for that.

	Irakli Gozalishvili [:irakli] [:gozala] [@gozala] Assignee
	Comment 20 • 10 years ago

Try commit https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed139eb11825

	Carsten Book [:Tomcat]
	Comment 21 • 10 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 22 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/4e9861bbc959

	Erik Vold [:erikvold] (please needinfo? me)
	Comment 23 • 10 years ago

Comment on attachment 8546315 [details]
Fix for on the SDK side

Where are the unload tests?

	Erik Vold [:erikvold] (please needinfo? me)
	Comment 24 • 10 years ago

(In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #23)
> Comment on attachment 8546315 [details]
> Fix for on the SDK side
> 
> Where are the unload tests?

I'd like to see some more tests which I've mentioned in the pull request.

	Irakli Gozalishvili [:irakli] [:gozala] [@gozala] Assignee
	Comment 25 • 10 years ago

Comment on attachment 8546315 [details]
Fix for on the SDK side

I have updated pull request to address Eriks comments. Although he's on PTO that is why I also asked ochameau in case he can get to this sooner.

	Alexandre Poirot [:ochameau]
	Comment 26 • 10 years ago

Comment on attachment 8546315 [details]
Fix for on the SDK side

Looks good to me, see my comment about listeners and chrome document test.

	[github robot]
	Comment 27 • 10 years ago

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/00fb5552f2a6cbe2c23fff824bfba39a03acfe7b
Merge pull request #1847 from Gozala/dev-panel-test-cleanup

Bug 1075490 Make sure tools created by tests are destroyed. r=erikvold

	Erik Vold [:erikvold] (please needinfo? me)
	Comment 28 • 10 years ago

Comment on attachment 8546315 [details]
Fix for on the SDK side

Looks like Alex can review this one!

	[github robot]
	Comment 29 • 9 years ago

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/26182ea9b9e00e9d3c7f0cfb084994ca4f491368
Merge pull request #1829 from Gozala/dev-panel-chrome

Bug 1075490 - Fix devtools docking. r=@ochameau

	Firefox Bug Husbandry Bot
	Comment 30 • 6 years ago

Removing leave-open keyword from resolved bugs, per :sylvestre.