Bug 95486 – PAGE_POPUP: Using window.pagePopupController will crash in non-V8 build

• Component: WebCore JavaScript
• Status: RESOLVED INVALID

[Yong Li]

Non-V8 build doesn't support V8EnabledPerContext. And it will access a null pointer in DOMWindowPagePopup::pagePopupController rather than returning "undefined"

[Yong Li]

Created attachment 161536 [details]
the patch

[Yong Li]

Ping Chromium team: any objections?

[Adam Barth]

Comment on attachment 161536 [details]
the patch

I believe this is needed to implement some <input> elements, like the date picker.  We should just fix the underlying crash.

[Kent Tamura]

I guess the crash happens with normal web pages because DOMWindow objects for normal web pages don't have DOMWindowPagePopup supplement objects.

Anyway, the right fix is to add support of EnabledPerContext to JSC binding.

[Yong Li]

(In reply to comment #4)
> I guess the crash happens with normal web pages because DOMWindow objects for normal web pages don't have DOMWindowPagePopup supplement objects.
> 
> Anyway, the right fix is to add support of EnabledPerContext to JSC binding.

Exactly. But before EnabledPerContext being supported, can we make it a V8 binding only property?

[Kent Tamura]

(In reply to comment #5)
> (In reply to comment #4)
> > I guess the crash happens with normal web pages because DOMWindow objects for normal web pages don't have DOMWindowPagePopup supplement objects.
> > 
> > Anyway, the right fix is to add support of EnabledPerContext to JSC binding.
> 
> Exactly. But before EnabledPerContext being supported, can we make it a V8 binding only property?

Why do you need to make it V8-only?
If you need no window.pagePopupController, you should exclude DOMWindowPagePopup.idl from build target.

[Yong Li]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > I guess the crash happens with normal web pages because DOMWindow objects for normal web pages don't have DOMWindowPagePopup supplement objects.
> > > 
> > > Anyway, the right fix is to add support of EnabledPerContext to JSC binding.
> > 
> > Exactly. But before EnabledPerContext being supported, can we make it a V8 binding only property?
> 
> Why do you need to make it V8-only?

Because it crashes on a non-V8 build.

> If you need no window.pagePopupController, you should exclude DOMWindowPagePopup.idl from build target.

DOMWindowPagePopup.idl is included in WebCore/CMakeLists.txt, which is a cross-platform file. I don't prefer to add platform-specific code to that cmake file. Any other solution?

[Adam Barth]

Why not just disable PAGE_POPUP in your port?

[Yong Li]

(In reply to comment #8)
> Why not just disable PAGE_POPUP in your port?

Because we are using ENABLE(PAGE_POPUP)

[Adam Barth]

I don't think ENABLE(PAGE_POPUP) works correctly without DOMWindowPagePopup.idl  and EnabledPerContext.  It seems that the proper resolution is to finish your implementation of ENABLE(PAGE_POPUP) by implementing EnabledPerContext.

[Yong Li]

(In reply to comment #10)
> I don't think ENABLE(PAGE_POPUP) works correctly without DOMWindowPagePopup.idl  and EnabledPerContext.  It seems that the proper resolution is to finish your implementation of ENABLE(PAGE_POPUP) by implementing EnabledPerContext.

What do you mean by "correctly"? We've got it work as we want, until it is broken by r118482. I think you guys should implement EnabledPerContext before including DOMWindowPagePopup.idl into the cross-platform cmake file.

[Adam Barth]

> What do you mean by "correctly"?

The pagePopupController object should be exposed to page popups, but not to arbitrary web sites.  For that to work, it needs to be enabled per context.  Simply ifdefing DOMWindowPagePopup out of the build will break ENABLE(PAGE_POPUP).  It has the same effect as simply disabling PAGE_POPUP.

[Anders Carlsson]

PAGE_POPUP was removed.