Closed Bug 983920 Opened 10 years ago Closed 10 years ago

Port window.sidebar and window.external to WebIDL

Categories

(Core :: DOM: Core & HTML, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

References

Details

Attachments

(2 files, 5 obsolete files)

      No description provided.
Attachment #8391641 - Flags: review?(bzbarsky)
Note that the reason I put the partial interface Window in Sidebar.webidl is that I couldn't test the MOZ_BUILD_APP variable in an #if inside Window.webidl.
Attachment #8391641 - Attachment is obsolete: true
Attachment #8391641 - Flags: review?(bzbarsky)
Comment on attachment 8391653 [details] [diff] [review]
Port window.sidebar and window.external to WebIDL; r=bzbarsky

https://tbpl.mozilla.org/?tree=Try&rev=063a8b8646b3
Attachment #8391653 - Flags: review?(bzbarsky)
You really do want the partial interface in Window.webidl.  We should disable the ability to put it somewhere else, since dependency tracking for WebIDL doesn't handle it right.  :(

I don't understand why you can't jsut do what what you did for the C++:

  if CONFIG['MOZ_BUILD_APP'] in ['browser', 'mobile/android']:
    DEFINES['HAVE_SIDEBAR'] = True

and then #ifdef HAVE_SIDEBAR in Window.webidl.
Ah, that #define should go into dom/bindings/moz.build, not dom/webidl/moz.build.  That was not intuitive at all!  :-)
Attachment #8391653 - Attachment is obsolete: true
Attachment #8391653 - Flags: review?(bzbarsky)
Comment on attachment 8391758 [details] [diff] [review]
Port window.sidebar and window.external to WebIDL; r=bzbarsky

Changes in this patch: marked the interface as NoInterfaceObject, marked the properties as Replaceable, and made them return the named child frames if one exists.

https://tbpl.mozilla.org/?tree=Try&rev=86e84f167346
Attachment #8391758 - Flags: review?(bzbarsky)
Attachment #8391758 - Attachment is obsolete: true
Attachment #8391758 - Flags: review?(bzbarsky)
Comment on attachment 8391760 [details] [diff] [review]
Port window.sidebar and window.external to WebIDL; r=bzbarsky

Fix the b2g build: https://tbpl.mozilla.org/?tree=Try&rev=642f9dff22cd
Attachment #8391760 - Flags: review?(bzbarsky)
Comment on attachment 8391760 [details] [diff] [review]
Port window.sidebar and window.external to WebIDL; r=bzbarsky

>+if CONFIG['MOZ_BUILD_APP'] in ['browser', 'mobile/android']:
>+    # This is needed for Window.webidl
>+    DEFINES['HAVE_SIDEBAR'] = True

I don't know whether any distos still ship FF-on-XR, but this looks like it would break them if they did.
(In reply to neil@parkwaycc.co.uk from comment #12)
> Comment on attachment 8391760 [details] [diff] [review]
> Port window.sidebar and window.external to WebIDL; r=bzbarsky
> 
> >+if CONFIG['MOZ_BUILD_APP'] in ['browser', 'mobile/android']:
> >+    # This is needed for Window.webidl
> >+    DEFINES['HAVE_SIDEBAR'] = True
> 
> I don't know whether any distos still ship FF-on-XR, but this looks like it
> would break them if they did.

Do such builds package the stuff in http://mxr.mozilla.org/mozilla-central/source/browser/installer/package-manifest.in?  If not, then they are already broken.  If they do, we can just change this to add 'xulrunner' to the build, and things will work.
Flags: needinfo?(neil)
Also, just for the record, since we have supported OpenSearch for years, we should probably deprecate and remove this API everywhere.
(In reply to Ehsan Akhgari from comment #13)
> Do such builds package the stuff in
> http://mxr.mozilla.org/mozilla-central/source/browser/installer/package-
> manifest.in?  If not, then they are already broken.  If they do, we can just
> change this to add 'xulrunner' to the build, and things will work.

Sorry, I don't have any idea what distros actually do, but FF-on-XR gets bandied around from time to time and I didn't want there to be heartache.
Flags: needinfo?(neil)
It turns out that the xulrunner packaging is broken (bug 984353).  But I will add 'xulrunner' to this list before landing not because I *know* what xulrunner does, but mostly out of the fear of the unknown.
FWIW xulrunner doesn't build anything in browser/, so it most likely has never had support for these two properties.  I am planning to land the patch without Neil's suggestion.
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #17)
> FWIW xulrunner doesn't build anything in browser/, so it most likely has
> never had support for these two properties.  I am planning to land the patch
> without Neil's suggestion.

Mossop, what do you think about this? ^
Flags: needinfo?(dtownsend+bugmail)
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #18)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #17)
> > FWIW xulrunner doesn't build anything in browser/, so it most likely has
> > never had support for these two properties.  I am planning to land the patch
> > without Neil's suggestion.
> 
> Mossop, what do you think about this? ^

XULRunner didn't, but firefox-on-xulrunner probably did looking at what you're doing because it used to declare those properties in nsSidebar.manifest. Are we actually getting to a place where the only way of defining custom DOM properties is in core code so third party apps can't do it themselves? That seems bad.

Realistically I don't know how many linux distros use FF-on-XR these days. It used to be all the major ones a few year ago but I think some have dropped off since then. Mike might be able to tell us how much of an issue this is now.
Flags: needinfo?(dtownsend+bugmail) → needinfo?(mh+mozilla)
The only way to define custom DOM properties that behave the way other DOM APIs behave and provide certain security guarantees is in core code, because it relies on per-property glue code that's compiled into the binary.

But also, we don't want random things defining custom DOM properties, because it fragments the web platform...

In any case, for now we still support the setup window.sidebar and window.external are using, clearly.  We may think about other ways to do it when we start thinking about actually removing the machinery it relies on, if we get there, but for now we're just focusing on making sure that everything shipped with Firefox by default follows the relevant specs and isn't a security footgun.
I guess I will add the interface to xulrunner builds here.
Attachment #8391760 - Attachment is obsolete: true
Attachment #8391760 - Flags: review?(bzbarsky)
Attachment #8395320 - Flags: review?(bzbarsky)
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #14)
> Also, just for the record, since we have supported OpenSearch for years, we
> should probably deprecate and remove this API everywhere.

window.external is the opensearch API, FWIW. Bug 862147 tracks getting rid of window.sidebar (and bug 518929 tracks implementing these in gecko to avoid various apps having to do it themselves...).
(In reply to Dave Townsend (:Mossop) from comment #19)
> Realistically I don't know how many linux distros use FF-on-XR these days.

Debian does, but not for long. IIRC Fedora used to, too, but I'm not sure they still do.
Flags: needinfo?(mh+mozilla) → needinfo?(stransky)
(In reply to comment #24)
> (In reply to Dave Townsend (:Mossop) from comment #19)
> > Realistically I don't know how many linux distros use FF-on-XR these days.
> 
> Debian does, but not for long. IIRC Fedora used to, too, but I'm not sure they
> still do.

Are there any docs on how to build Firefox on XULRunner somewhere so that I can test to see whether this patch breaks it or not?
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #25)
> Are there any docs on how to build Firefox on XULRunner somewhere so that I
> can test to see whether this patch breaks it or not?

http://glandium.org/blog/?p=1258

Note it's already broken by bug 976002
(In reply to Mike Hommey [:glandium] from comment #26)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #25)
> > Are there any docs on how to build Firefox on XULRunner somewhere so that I
> > can test to see whether this patch breaks it or not?
> 
> http://glandium.org/blog/?p=1258

Note that's very old, and you don't need most of these options anymore.

The following should be enough nowadays:
mk_add_options MOZ_BUILD_PROJECTS="xulrunner browser"
ac_add_app_options browser --enable-application=browser
ac_add_app_options browser --enable-default-toolkit=cairo-gtk2
ac_add_app_options browser --with-libxul-sdk=../xulrunner/dist
ac_add_app_options xulrunner --enable-application=xulrunner

(change the default toolkit to the toolkit for your platform)
(In reply to comment #26)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #25)
> > Are there any docs on how to build Firefox on XULRunner somewhere so that I
> > can test to see whether this patch breaks it or not?
> 
> http://glandium.org/blog/?p=1258
> 
> Note it's already broken by bug 976002

Sigh..  OK I give up here.  I've done the best I could here, and if Firefox on XULRunner supports the same XPCOM components under browser/ as Firefox does (without which this functionality would be broken in the first place!) then everything should just work fine out of the box after this lands.  If not, then people can file follow-up bugs with more details and I'll help there.
Fedora doesn't use FF-on-XR anymore.
Flags: needinfo?(stransky)
Comment on attachment 8395320 [details] [diff] [review]
Port window.sidebar and window.external to WebIDL; r=bzbarsky

>+++ b/dom/base/moz.build

Shouldn't HAVE_SIDEBAR be defined for xulrunner?  The binding bits are....

>+++ b/dom/base/nsGlobalWindow.cpp

Why not just go head and make mSidebar an nsRefPtr<Sidebar>?  Is the issue that then you'd have to ifdef it in the header or something?  Can you just forward-declare in the header and then just ifdef the CC bits in the .cpp?

If it has to be an nsISupports, please document why it does.

>+                                     OwningSidebarOrNullOrWindowProxy& aResult,

I don't see how this function can ever return null; seems to me like the union should have "Sidebar" in it, not "Sidebar?".

>+  // First check for a named frame named "content" or "external"

You mean "sidebar" or "external".

But also, we shouldn't do this for "external".  That's in the spec and all, and returns an instance of External, and that's what Chrome, say, implements.  So there ought not to be things depending on window.external beinga named frame.

For window.sidebar, we will sadly need this hackery.  :(

Also, note that since the interface is called "External" in the spec, we should call it that; it'll just have an extra method on it (which should probably go on a partial interface documented as a Mozilla extension).

>+nsGlobalWindow::GetSidebarOrExternal(JSContext* cx,

The only thing this cx is used for is rooting.  Just nix it and do "AutoJSContext cx" on the stack when we need it.

>+    if (!aRv.Failed()) {
>+      mSidebar = new Sidebar(jsImplObj, this);
>+    }

How about:

  if (aRv.Failed()) {
    return;
  }
  mSidebar = new Sidebar(jsImplObj, this);

and then when you exit this block you unconditionally know you have a non-null mSidebar.  Which is why we know this method can never successfully return null.

>+++ b/dom/base/nsGlobalWindow.h

The new methods here should be ifdefed, right?  I guess that gets you that problem with ifdefs in the header...

So maybe they should be unconditional, and the impls should exist unconditionally, but they should throw when called if not HAVE_SIDEBAR?

>+++ b/dom/bindings/BindingUtils.cpp

>+  return window.forget();

That changes the behavior.  Before, we always returned null when aRv.Failed().

I guess the one current caller correctly checks aRv before touching the return value, so this is ok.

>+++ b/dom/bindings/Bindings.conf

Per above, don't need the change here.

>+++ b/dom/webidl/Sidebar.webidl

External.webidl

>+ NoInterfaceObject]

Drop that, since External should have an interface object.

>+  readonly attribute (Sidebar? or WindowProxy) external;

There's already a line for that above, commented out.  Just uncomment it and make it ifdeffed?

The rest look fine, but I'd like to see an updated patch.
Attachment #8395320 - Flags: review?(bzbarsky) → review-
(In reply to Boris Zbarsky [:bz] from comment #30)
> >+++ b/dom/base/nsGlobalWindow.cpp
> 
> Why not just go head and make mSidebar an nsRefPtr<Sidebar>?  Is the issue
> that then you'd have to ifdef it in the header or something?  Can you just
> forward-declare in the header and then just ifdef the CC bits in the .cpp?
> 
> If it has to be an nsISupports, please document why it does.

Making it an nsRefPtr<Sidebar> means that nsGlobalWindow.cpp will require to see the definition of ~Sidebar even on places where that class doesn't exist (b2g).  I'll document this.

> >+                                     OwningSidebarOrNullOrWindowProxy& aResult,
> 
> I don't see how this function can ever return null; seems to me like the
> union should have "Sidebar" in it, not "Sidebar?".

It can return null on XULRunner when the "@mozilla.org/sidebar;1" component doesn't exist (which is the default on XULRunner.)

> >+  // First check for a named frame named "content" or "external"
> 
> You mean "sidebar" or "external".
> 
> But also, we shouldn't do this for "external".  That's in the spec and all,
> and returns an instance of External, and that's what Chrome, say,
> implements.  So there ought not to be things depending on window.external
> beinga named frame.

Oh, OK.  We have a test which tests this, I'll fix the test then.

> For window.sidebar, we will sadly need this hackery.  :(
> 
> Also, note that since the interface is called "External" in the spec, we
> should call it that; it'll just have an extra method on it (which should
> probably go on a partial interface documented as a Mozilla extension).

Ah, so honestly I didn't pay attention to the fact that External is spec'ed.  So now, given that we don't have an implementation of the sidebar component on b2g, we have a few options:

1. Make b2g not have window.external at all.  This is the status quo.
2. Make b2g have it but return null, or throw, or both, when you access it.

I prefer #1.  What about you?

Also, given the fact that window.external/sidebar both return null in XULRunner, do you think we should lie in the IDL about that?

> >+    if (!aRv.Failed()) {
> >+      mSidebar = new Sidebar(jsImplObj, this);
> >+    }
> 
> How about:
> 
>   if (aRv.Failed()) {
>     return;
>   }
>   mSidebar = new Sidebar(jsImplObj, this);
> 
> and then when you exit this block you unconditionally know you have a
> non-null mSidebar.  Which is why we know this method can never successfully
> return null.

Oh, so you mean, throw instead of return null?

> >+++ b/dom/base/nsGlobalWindow.h
> 
> The new methods here should be ifdefed, right?  I guess that gets you that
> problem with ifdefs in the header...

Indeed.

> So maybe they should be unconditional, and the impls should exist
> unconditionally, but they should throw when called if not HAVE_SIDEBAR?

Like I said above, I prefer to hide these names on b2g than to make them visible but inaccessible by throwing.  For XULRunner, I don't care much either way.

> >+++ b/dom/bindings/BindingUtils.cpp
> 
> >+  return window.forget();
> 
> That changes the behavior.  Before, we always returned null when
> aRv.Failed().
> 
> I guess the one current caller correctly checks aRv before touching the
> return value, so this is ok.

Good point, I'll fix it anyway.
Flags: needinfo?(bzbarsky)
> It can return null on XULRunner when the "@mozilla.org/sidebar;1" component doesn't exist 

How would it do that?  We'd call ConstructJSImplementation(), then do:

     nsCOMPtr<nsISupports> implISupports = do_CreateInstance(aContractId);

and then null-check that and do aRv.Throw(NS_ERROR_FAILURE).  This will propagate up, since the caller passes in the binding-provided ErrorResult here, and we'll throw an exception to the web page.

If we are _trying_ to return null in this case, instead of throwing an exception, then we need to rejigger the code a bit, no?

> I prefer #1.  What about you?

I prefer #1 as well.  And a followup to implement .external on b2g or a bug on the spec to move .external to a separate (optional) spec or make it nullable or something?

> Oh, so you mean, throw instead of return null?

That's what the code does with your patch as-is, yes.

> I prefer to hide these names on b2g than to make them visible but inaccessible by
> throwing.

There's a difference between whether the JS names are exposed and what the C++ does.

The patch as it stands has C++ code that's unconditional calling a function that is declared unconditionally but only defined if HAVE_SIDEBAR.  I expect that to cause link failures in at least some compilers.

What I'm proposing is that on the C++ side we have the declarations and definitions unconditional for all this stuff, but the body of the function conditioned on HAVE_SIDEBAR to either throw or do something useful.  Then we also condition the binding code on HAVE_SIDEBAR so that if it's not defined the binding won't ever call into the (throwing in this case) C++.  Make sense?
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #32)
> > It can return null on XULRunner when the "@mozilla.org/sidebar;1" component doesn't exist 
> 
> How would it do that?  We'd call ConstructJSImplementation(), then do:
> 
>      nsCOMPtr<nsISupports> implISupports = do_CreateInstance(aContractId);
> 
> and then null-check that and do aRv.Throw(NS_ERROR_FAILURE).  This will
> propagate up, since the caller passes in the binding-provided ErrorResult
> here, and we'll throw an exception to the web page.
> 
> If we are _trying_ to return null in this case, instead of throwing an
> exception, then we need to rejigger the code a bit, no?

You're right, yes!

> > I prefer #1.  What about you?
> 
> I prefer #1 as well.  And a followup to implement .external on b2g or a bug
> on the spec to move .external to a separate (optional) spec or make it
> nullable or something?

I guess we need to support this in b2g at some point.  Bug 897445 is already on file for adding support for OpenSearch itself, commented there...

> > I prefer to hide these names on b2g than to make them visible but inaccessible by
> > throwing.
> 
> There's a difference between whether the JS names are exposed and what the
> C++ does.
> 
> The patch as it stands has C++ code that's unconditional calling a function
> that is declared unconditionally but only defined if HAVE_SIDEBAR.  I expect
> that to cause link failures in at least some compilers.

It will cause link failures if you try to call that function.

> What I'm proposing is that on the C++ side we have the declarations and
> definitions unconditional for all this stuff, but the body of the function
> conditioned on HAVE_SIDEBAR to either throw or do something useful.  Then we
> also condition the binding code on HAVE_SIDEBAR so that if it's not defined
> the binding won't ever call into the (throwing in this case) C++.  Make
> sense?

Well, the thing is, OwningSidebarOrNullOrWindowProxy is forward-declared and defined in the bindings generated code, so I couldn't touch it in the !defined(HAVE_SIDEBAR) case.  However, if I only try to throw an exception through the ErrorResult argument, then it should work fine.  So I'll do that.
> However, if I only try to throw an exception through the ErrorResult argument

Yes, exactly.
Comment on attachment 8398286 [details] [diff] [review]
Port window.sidebar and window.external to WebIDL; r=bzbarsky

One thing to note is that I made window.external replaceable.  Without that, these reftests break: content/test/reftest/bug591981*

I tested Chrome and it has the same behavior.  If you think this makes sense, I'd be happy to file a spec bug for this.
Attachment #8398286 - Flags: review?(bzbarsky)
Comment on attachment 8398286 [details] [diff] [review]
Port window.sidebar and window.external to WebIDL; r=bzbarsky

Argh, wrong patch!
Attachment #8398286 - Attachment is obsolete: true
Attachment #8398286 - Flags: review?(bzbarsky)
Attachment #8398287 - Flags: review?(bzbarsky)
Comment on attachment 8398287 [details] [diff] [review]
Port window.sidebar and window.external to WebIDL; r=bzbarsky

GetSidebarOrExternal could just be called GetExternal(), no?  It'll mean an extra forward-to-inner in GetSidebar(), but that's ok.

r=me with that nit.  Making .external replaceable definitely makes sense; please file the spec bug.  

Thank you for fixing up this stuff!
Attachment #8398287 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/d9e6a6c40a57
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.