Bug 82664 – Need DOMWindow mechanism to supplement UserScripts for page cache notifications

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Brady Eidson]

Need DOMWindow mechanism to supplement UserScripts for page cache notifications.
---
Clients of the user script mechanism often also inject native hooks into the javascript context for those user scripts to interact with.

When a page is suspended for the page cache and then restored from the page cache, those injected hooks are cleared, leading to many types of bugs with the user scripts.

Such clients need to have notifications for when the global object (the DOMWindow) is suspended for the page cache, resumed from the page cache, and destroyed - whether or not in the page cache.

DOMWindowProperty gives us a fantastic hook for this, and we create a "DOMWindowExtension" for each user script we inject into a world to uniquely track the lifetime of each world in each DOMWindow.

The mechanism has room to be extended in the future to provide more functionality than the "unique-identifier plus notifications" included in this first pass.

WebCore implementation, WebKit2 API, and WebKit2 API Test forthcoming.

In radar as <rdar://problem/10120155>

[Brady Eidson]

Created attachment 134689 [details]
Patch v1 - Add the API and an API test

[WebKit Review Bot]

Attachment 134689 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit2/WebProcess/InjectedBundle/InjectedBundleClient.h:49:  The parameter name "extension" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/page/Frame.cpp:42:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/WebProcess/InjectedBundle/InjectedBundleDOMWindowExtension.h:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Tools/TestWebKitAPI/InjectedBundleController.h:61:  The parameter name "bundle" adds no information, so it should be removed.  [readability/parameter_name] [5]
Tools/TestWebKitAPI/InjectedBundleController.h:61:  The parameter name "extension" adds no information, so it should be removed.  [readability/parameter_name] [5]
Tools/TestWebKitAPI/InjectedBundleController.h:62:  The parameter name "bundle" adds no information, so it should be removed.  [readability/parameter_name] [5]
Tools/TestWebKitAPI/InjectedBundleController.h:62:  The parameter name "extension" adds no information, so it should be removed.  [readability/parameter_name] [5]
Tools/TestWebKitAPI/InjectedBundleController.h:63:  The parameter name "bundle" adds no information, so it should be removed.  [readability/parameter_name] [5]
Tools/TestWebKitAPI/InjectedBundleController.h:63:  The parameter name "extension" adds no information, so it should be removed.  [readability/parameter_name] [5]
Tools/TestWebKitAPI/InjectedBundleController.h:64:  The parameter name "bundle" adds no information, so it should be removed.  [readability/parameter_name] [5]
Tools/TestWebKitAPI/InjectedBundleController.h:64:  The parameter name "extension" adds no information, so it should be removed.  [readability/parameter_name] [5]
Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp:95:  One space before end of line comments  [whitespace/comments] [5]
Tools/TestWebKitAPI/Tests/WebKit2/DOMWindowExtensionBasic_Bundle.cpp:56:  The parameter name "frame" adds no information, so it should be removed.  [readability/parameter_name] [5]
Tools/TestWebKitAPI/Tests/WebKit2/DOMWindowExtensionBasic_Bundle.cpp:59:  The parameter name "bundle" adds no information, so it should be removed.  [readability/parameter_name] [5]
Tools/TestWebKitAPI/Tests/WebKit2/DOMWindowExtensionBasic_Bundle.cpp:59:  The parameter name "extension" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 15 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 134689 [details]
Patch v1 - Add the API and an API test

Attachment 134689 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12243005

[Brady Eidson]

(In reply to comment #3)
> (From update of attachment 134689 [details])
> Attachment 134689 [details] did not pass win-ews (win):
> Output: http://queues.webkit.org/results/12243005

I completely overlooked other WebCore builds since this is really to support a Mac WK2 feature.  My bad!  New patch is on its way.

[Early Warning System Bot]

Comment on attachment 134689 [details]
Patch v1 - Add the API and an API test

Attachment 134689 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12207029

[Brady Eidson]

Created attachment 134699 [details]
Patch v2 - Now with more build fixes!

[WebKit Review Bot]

Attachment 134699 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1
Source/WebKit2/WebProcess/InjectedBundle/InjectedBundleClient.h:49:  The parameter name "extension" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/WebProcess/InjectedBundle/InjectedBundleDOMWindowExtension.h:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp:95:  One space before end of line comments  [whitespace/comments] [5]
Tools/TestWebKitAPI/InjectedBundleController.h:61:  The parameter name "bundle" adds no information, so it should be removed.  [readability/parameter_name] [5]
Tools/TestWebKitAPI/InjectedBundleController.h:61:  The parameter name "extension" adds no information, so it should be removed.  [readability/parameter_name] [5]
Tools/TestWebKitAPI/InjectedBundleController.h:62:  The parameter name "bundle" adds no information, so it should be removed.  [readability/parameter_name] [5]
Tools/TestWebKitAPI/InjectedBundleController.h:62:  The parameter name "extension" adds no information, so it should be removed.  [readability/parameter_name] [5]
Tools/TestWebKitAPI/InjectedBundleController.h:63:  The parameter name "bundle" adds no information, so it should be removed.  [readability/parameter_name] [5]
Tools/TestWebKitAPI/InjectedBundleController.h:63:  The parameter name "extension" adds no information, so it should be removed.  [readability/parameter_name] [5]
Tools/TestWebKitAPI/InjectedBundleController.h:64:  The parameter name "bundle" adds no information, so it should be removed.  [readability/parameter_name] [5]
Tools/TestWebKitAPI/InjectedBundleController.h:64:  The parameter name "extension" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/ChangeLog:50:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:51:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:52:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:53:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:54:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Tools/TestWebKitAPI/Tests/WebKit2/DOMWindowExtensionBasic_Bundle.cpp:56:  The parameter name "frame" adds no information, so it should be removed.  [readability/parameter_name] [5]
Tools/TestWebKitAPI/Tests/WebKit2/DOMWindowExtensionBasic_Bundle.cpp:59:  The parameter name "bundle" adds no information, so it should be removed.  [readability/parameter_name] [5]
Tools/TestWebKitAPI/Tests/WebKit2/DOMWindowExtensionBasic_Bundle.cpp:59:  The parameter name "extension" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 19 in 41 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 134699 [details]
Patch v2 - Now with more build fixes!

Attachment 134699 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12239015

[Sam Weinig]

Comment on attachment 134699 [details]
Patch v2 - Now with more build fixes!

I am not sold on this API, specially the one extension per world. I am going to take some time to look it over.

[Brady Eidson]

(In reply to comment #9)
> (From update of attachment 134699 [details])
> I am not sold on this API, specially the one extension per world. I am going to take some time to look it over.

Would love to talk with you about your concerns in person.  It went through a few iterations here before we settled on something that fulfilled the use cases, followed the user script model, and was efficient.

[Brady Eidson]

And - like a bonehead - forgot to add new files to the other WK2 builds.  Will do so in the morning.

[Early Warning System Bot]

Comment on attachment 134699 [details]
Patch v2 - Now with more build fixes!

Attachment 134699 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12262011

[Collabora GTK+ EWS bot]

Comment on attachment 134699 [details]
Patch v2 - Now with more build fixes!

Attachment 134699 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12253039

[Brady Eidson]

Created attachment 134827 [details]
Patch v3 - Now with even more project files!

[Jessie Berlin]

Comment on attachment 134827 [details]
Patch v3 - Now with even more project files!

View in context: https://bugs.webkit.org/attachment.cgi?id=134827&action=review

Holding off on reviewing it because Sam Weinig wants to talk further about the design.

> Tools/MiniBrowser/mac/WebBundle/WebBundleMain.m:105
> +        0  // didDestroyDOMWindowExtension

Shouldn't you be bumping the version of the client here as well?

> Tools/TestWebKitAPI/Tests/WebKit2/DOMWindowExtensionBasic.cpp:2
> + * Copyright (C) 2010 Apple Inc. All rights reserved.

I think you meant 2012 here.

> Tools/TestWebKitAPI/Tests/WebKit2/DOMWindowExtensionBasic_Bundle.cpp:2
> + * Copyright (C) 2010 Apple Inc. All rights reserved.

I think you meant 2012.

> Tools/TestWebKitAPI/Tests/WebKit2/DOMWindowExtensionBasic_Bundle.cpp:78
> +    ((DOMWindowExtensionBasic*)clientInfo)->frameLoadFinished(frame);

Can this be a reinterpret cast instead?

[Brady Eidson]

(In reply to comment #15)
> (From update of attachment 134827 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134827&action=review
> 
> Holding off on reviewing it because Sam Weinig wants to talk further about the design.
> 
> > Tools/MiniBrowser/mac/WebBundle/WebBundleMain.m:105
> > +        0  // didDestroyDOMWindowExtension
> 
> Shouldn't you be bumping the version of the client here as well?

It's already using kWKBundleClientCurrentVersion.  That's why it had to be updated to keep its build working.

> 
> > Tools/TestWebKitAPI/Tests/WebKit2/DOMWindowExtensionBasic.cpp:2
> > + * Copyright (C) 2010 Apple Inc. All rights reserved.
> 
> I think you meant 2012 here.
> 
> > Tools/TestWebKitAPI/Tests/WebKit2/DOMWindowExtensionBasic_Bundle.cpp:2
> > + * Copyright (C) 2010 Apple Inc. All rights reserved.
> 
> I think you meant 2012.

Yuppers.

> 
> > Tools/TestWebKitAPI/Tests/WebKit2/DOMWindowExtensionBasic_Bundle.cpp:78
> > +    ((DOMWindowExtensionBasic*)clientInfo)->frameLoadFinished(frame);
> 
> Can this be a reinterpret cast instead?

There certainly doesn't seem to be an established style throughout TestWebKitAPI.  It sure can be.

[Dimitri Glazkov (Google)]

cc'ing Adam and Haraken, since they are frolicking in the same meadow (or nearby).

[Brady Eidson]

Comment on attachment 134827 [details]
Patch v3 - Now with even more project files!

Chatted with Sam and Jessie, going a direction slightly tweaked from this API.  New patch in the works

[Brady Eidson]

Created attachment 136358 [details]
Patch v4 - Explicit creation of DOMWindowExtensions

[WebKit Review Bot]

Attachment 136358 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1
Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageGroupClient.h:34:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Tools/TestWebKitAPI/Tests/WebKit2/DOMWindowExtensionBasic_Bundle.cpp:41:  The parameter name "frame" adds no information, so it should be removed.  [readability/parameter_name] [5]
Tools/TestWebKitAPI/Tests/WebKit2/DOMWindowExtensionBasic_Bundle.cpp:76:  The parameter name "frame" adds no information, so it should be removed.  [readability/parameter_name] [5]
Tools/TestWebKitAPI/Tests/WebKit2/DOMWindowExtensionBasic_Bundle.cpp:146:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 4 in 42 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 136358 [details]
Patch v4 - Explicit creation of DOMWindowExtensions

Attachment 136358 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12371552

[Build Bot]

Comment on attachment 136358 [details]
Patch v4 - Explicit creation of DOMWindowExtensions

Attachment 136358 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12373508

[Brady Eidson]

Yep definitely need to add one more new file to to the other ports' WK2.

Will do so in the A.M.

Reviewing the rest of the patch in the meantime is *not* wasted effort...

[Brady Eidson]

Created attachment 136466 [details]
Patch v5 - More project file additions

[Sam Weinig]

Comment on attachment 136466 [details]
Patch v5 - More project file additions

View in context: https://bugs.webkit.org/attachment.cgi?id=136466&action=review

Looking better, still needs some work.

> Source/WebCore/loader/FrameLoaderClient.h:336
> +        virtual void dispatchCanCreateGlobalObject(DOMWrapperWorld*) { }

I don't really understand the naming here.  The client can't really create a global object can it?

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePageGroup.h:50
> +    WKBundlePageGroupDidCreateGlobalObjectForFrameCallback                      didCreateGlobalObjectForFrame;
> +    WKBundlePageGroupWillDisconnectDOMWindowExtensionFromGlobalObjectCallback   willDisconnectDOMWindowExtensionFromGlobalObject;
> +    WKBundlePageGroupDidReconnectDOMWindowExtensionToGlobalObjectCallback       didReconnectDOMWindowExtensionToGlobalObject;
> +    WKBundlePageGroupWillDestroyGlobalObjectForDOMWindowExtensionCallback       willDestroyGlobalObjectForDOMWindowExtension;
> +};

I think these should go on the BundlePageLoadClient as discussed.

[Brady Eidson]

(In reply to comment #25)
> (From update of attachment 136466 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136466&action=review
> 
> Looking better, still needs some work.
> 
> > Source/WebCore/loader/FrameLoaderClient.h:336
> > +        virtual void dispatchCanCreateGlobalObject(DOMWrapperWorld*) { }
> 
> I don't really understand the naming here.  The client can't really create a global object can it?

Actually it can.  The global object is created on demand, and the client can create it by demanding it.

In this case the client is WebKit2 which translates it to "didCreateGlobalObject" after having created it.

Perhaps "isReadyToAccessGlobalObject"?  Some other variant?

> 
> > Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePageGroup.h:50
> > +    WKBundlePageGroupDidCreateGlobalObjectForFrameCallback                      didCreateGlobalObjectForFrame;
> > +    WKBundlePageGroupWillDisconnectDOMWindowExtensionFromGlobalObjectCallback   willDisconnectDOMWindowExtensionFromGlobalObject;
> > +    WKBundlePageGroupDidReconnectDOMWindowExtensionToGlobalObjectCallback       didReconnectDOMWindowExtensionToGlobalObject;
> > +    WKBundlePageGroupWillDestroyGlobalObjectForDOMWindowExtensionCallback       willDestroyGlobalObjectForDOMWindowExtension;
> > +};
> 
> I think these should go on the BundlePageLoadClient as discussed.

Indeed.

[Brady Eidson]

Created attachment 138587 [details]
Patch v6 - FrameLoaderClient method name changed, and WK2 client methods moved to BundlePageLoaderClient

[Brady Eidson]

Landed in http://trac.webkit.org/changeset/115083

[Csaba Osztrogonác]

Reopen, because it broke the Qt-WK2 build:

cc1plus: warnings being treated as errors
/home/webkitbuildbot/slaves/release32bit-qt5/buildslave/qt-linux-32-release-qt5/build/Source/WebKit2/WebProcess/qt/QtBuiltinBundlePage.cpp: In constructor ‘WebKit::QtBuiltinBundlePage::QtBuiltinBundlePage(WebKit::QtBuiltinBundle*, const OpaqueWKBundlePage*)’:
/home/webkitbuildbot/slaves/release32bit-qt5/buildslave/qt-linux-32-release-qt5/build/Source/WebKit2/WebProcess/qt/QtBuiltinBundlePage.cpp:73: error: missing initializer for member ‘WKBundlePageLoaderClient::didCreateGlobalObjectForFrame’
/home/webkitbuildbot/slaves/release32bit-qt5/buildslave/qt-linux-32-release-qt5/build/Source/WebKit2/WebProcess/qt/QtBuiltinBundlePage.cpp:73: error: missing initializer for member ‘WKBundlePageLoaderClient::willDisconnectDOMWindowExtensionFromGlobalObject’
/home/webkitbuildbot/slaves/release32bit-qt5/buildslave/qt-linux-32-release-qt5/build/Source/WebKit2/WebProcess/qt/QtBuiltinBundlePage.cpp:73: error: missing initializer for member ‘WKBundlePageLoaderClient::didReconnectDOMWindowExtensionToGlobalObject’
/home/webkitbuildbot/slaves/release32bit-qt5/buildslave/qt-linux-32-release-qt5/build/Source/WebKit2/WebProcess/qt/QtBuiltinBundlePage.cpp:73: error: missing initializer for member ‘WKBundlePageLoaderClient::willDestroyGlobalObjectForDOMWindowExtension’

[Jesus Sanchez-Palencia]

(In reply to comment #28)
> Landed in http://trac.webkit.org/changeset/115083

As warned by qt-wk2 ews, this broke the Qt bot and it was kept like that for a few rounds of patches, not even mentioning the style issues also warned by the style ews... I have landed a fix for the Qt build issue in http://trac.webkit.org/changeset/115095 but it seems there is still some layout failures for us after this. Can you please have a look at it, Brady?

[Csaba Osztrogonác]

Thanks for the buildfix Jeez.

I think the svg fails are unrelated to this bug, http://trac.webkit.org/changeset/115085 is the culprit. Maybe we only need to update them.

[Brady Eidson]

Apologies for the breakage.  I - admittedly - didn't wait for ews to finish as the last version passed ews, yet I didn't take in to considersation that the one substantive change might break another build.

The style errors are bogus as they don't account for well established style within TestWebKitAPI.

I'll see what I can find about the qt layouttest failures...  they're almost certainly inconsequential based on the change...

[Brady Eidson]

(In reply to comment #31)
> Thanks for the buildfix Jeez.
> 
> I think the svg fails are unrelated to this bug, http://trac.webkit.org/changeset/115085 is the culprit. Maybe we only need to update them.

Yup, sure seems like it.

I had a fix ready to check in but Jeez beat me to it.

Sorry for the noise, guys.

[Csaba Osztrogonác]

(In reply to comment #32)
> Apologies for the breakage.  I - admittedly - didn't wait for ews to finish as the last version passed ews, yet I didn't take in to considersation that the one substantive change might break another build.

Not a big problem, the build was fixed quickly. And it show us that Qt 
EWS bots are too slow in rush hours, so we should setup more bots.
 
> The style errors are bogus as they don't account for well established style within TestWebKitAPI.
Is there a bug report to fix the style checker?