WebKit Bugzilla
As a follow-up from the fix on bug 53648, it's still possible to execute JavaScript in a frame after active DOM objects have been suspended. Given this frame hierarchy: Main frame frame A frame B When navigating away from the main frame (and putting it in the page cache via CachedFrame), we still do things in this order: 1. Fire pagehide for the main frame 2. Fire pagehide for frame A 3. Suspend active DOM objects for frame A 4. Fire pagehide for frame B 5. Suspend active DOM objects for frame B 6. Suspend active DOM objects for the main frame If frame B in its pagehide handler calls a function in frame A that uses setTimeout, that timer will not be suspended, and could fire while in the page cache. The correct fix would be to separate the recursion steps; one full pass to fire pagehide and another to suspend active DOM objects. This may require some refactoring of CachedFrame::CachedFrame, where all this happens currently.
Might this be the reason for http://build.webkit.org/results/GTK Linux 32-bit Release/r77615 (10360)/fast/events/pagehide-timeout-pretty-diff.html ?
(In reply to comment #1) > Might this be the reason for > > http://build.webkit.org/results/GTK Linux 32-bit Release/r77615 (10360)/fast/events/pagehide-timeout-pretty-diff.html No, I don't think this is the reason. The test failure makes it look like the GTK port is not getting the page cache enabled for this test, even though the test is requesting it. Filed bug 53771 about it.
This will be fixed by that patch for https://bugs.webkit.org/show_bug.cgi?id=76063
(In reply to comment #3) > This will be fixed by that patch for https://bugs.webkit.org/show_bug.cgi?id=76063 Why this assertion? 76063 repeatedly purports to be about WebKit2 API only yet this is a cross WebProcess-only crossplatform WebCore bug.
(In reply to comment #4) > (In reply to comment #3) > > This will be fixed by that patch for https://bugs.webkit.org/show_bug.cgi?id=76063 > > Why this assertion? > > 76063 repeatedly purports to be about WebKit2 API only yet this is a cross WebProcess-only crossplatform WebCore bug. Reading through 76063 I now see that it is was more complicated in scope than the claimed "Add a new WK2 API" task. *sigh*
(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > This will be fixed by that patch for https://bugs.webkit.org/show_bug.cgi?id=76063 > > > > Why this assertion? > > > > 76063 repeatedly purports to be about WebKit2 API only yet this is a cross WebProcess-only crossplatform WebCore bug. > > Reading through 76063 I now see that it is was more complicated in scope than the claimed "Add a new WK2 API" task. *sigh* With the API old bugs were exposed and fixed.
(In reply to comment #6) > (In reply to comment #5) > > (In reply to comment #4) > > > (In reply to comment #3) > > > > This will be fixed by that patch for https://bugs.webkit.org/show_bug.cgi?id=76063 > > > > > > Why this assertion? > > > > > > 76063 repeatedly purports to be about WebKit2 API only yet this is a cross WebProcess-only crossplatform WebCore bug. > > > > Reading through 76063 I now see that it is was more complicated in scope than the claimed "Add a new WK2 API" task. *sigh* > > With the API old bugs were exposed and fixed. It should be noted that without the new API it is hard to trigger cases of this bug, making it is practically impossible to test, and I am not going to submit patches without having tested them first. So the API is required.
(In reply to comment #6) > (In reply to comment #5) > > Reading through 76063 I now see that it is was more complicated in scope than the claimed "Add a new WK2 API" task. *sigh* > > With the API old bugs were exposed and fixed. With the new API you exposed new ways of reproducing/noticing old bugs. With the WebCore changes you fixed them. (In reply to comment #7) > It should be noted that without the new API it is hard to trigger cases of this bug, making it is practically impossible to test, and I am not going to submit patches without having tested them first. So the API is required. Mihai illustrated pure web content that would reproduce this bug as far back as a year ago. Even if it's not 100% repro, it's better than no test coverage. I'm also not confused as to why you're unwilling to submit a patch without test coverage here, yet your patch in 76063 has no test coverage. Besides not having a test is not an automatic disqualifier as our patch guidelines state "If no layout test can be (or needs to be) constructed for the fix, you must explain why a new test isn't necessary" We like to keep a 1:1 relationship between patches and bugs as it makes it easier to track changes in behavior later, easier to roll out smaller changes, easier to review smaller patches, etc etc. This policy is to make everyone's life easier. Like it or not your patch in 76063 *DOES* do two logically separate things. It fixes a cross platform WebCore bug and it adds new WebKit2 API. Please reconsider splitting out the WebCore-only portion here,
(In reply to comment #8) > Like it or not your patch in 76063 *DOES* do two logically separate things. It fixes a cross platform WebCore bug and it adds new WebKit2 API. > I do not think it does two separate things. If this bug did not exist it would not have been an issue. It was pure accident I stumbled across this bug in the code comments. But for bugzilla clarity I wouldn't mind separating the patch. I do like to do at least manual tests, but this has been done, and the code is clear enough. But please note that the last remaining reviewer comments I have against my patch (by Simon Fraser), are on the parts that would be a submitted here instead.
(In reply to comment #9) > (In reply to comment #8) > > Like it or not your patch in 76063 *DOES* do two logically separate things. It fixes a cross platform WebCore bug and it adds new WebKit2 API. > > > > I do not think it does two separate things. Yes it does. 1 - It fixes this long standing bug in WebCore, a task tracked here in bug 53733 2 - It adds new WebKit2 API giving the embedding application the ability to forcefully "pause" the web page, a task tracked by 76063 > If this bug did not exist it would not have been an issue. If you're saying that if this bug existed then the separate task of adding new WebKit 2 API wouldn't be necessary then we should close 76063 as Invalid and just fix this bug! >It was pure accident I stumbled across this bug in the code comments. But for bugzilla clarity I wouldn't mind separating the patch. I do like to do at least manual tests, but this has been done, and the code is clear enough. > But please note that the last remaining reviewer comments I have against my patch (by Simon Fraser), are on the parts that would be a submitted here instead. That's fine, and he won't mind. And I plan on thoroughly reviewing the patch here from scratch. Thanks!
(In reply to comment #10) > > If you're saying that if this bug existed then the separate task of adding new WebKit 2 API wouldn't be necessary then we should close 76063 as Invalid and just fix this bug! Sorry I meant to say: If you're saying that if this bug DID NOT EXIST then the separate task of adding new WebKit 2 API wouldn't be necessary, then we should close 76063 as Invalid and just fix this bug!
(In reply to comment #8) > Mihai illustrated pure web content that would reproduce this bug as far back as a year ago. I didn't actually create a test case for this bug (only bug 53648, which is related), but it should be easy to make a test case for this bug too. See http://persistent.info/webkit/test-cases/webkit-53733/main.html ( (it will alert "I am running in a.html" even after navigating away to dummy.html)
(In reply to comment #12) > I didn't actually create a test case for this bug (only bug 53648, which is related), but it should be easy to make a test case for this bug too. See http://persistent.info/webkit/test-cases/webkit-53733/main.html (it will alert "I am running in a.html" even after navigating away to dummy.html) Just to be clear, that URL is a test for this bug, it should be easy to convert to a layout test (see the test case in http://trac.webkit.org/changeset/77559, the test logs when various bits of code execute, so the ordering of the log statements makes it easy to verify that no code execution is taking place while the page is in the page cache).
(In reply to comment #11) > (In reply to comment #10) > > > > If you're saying that if this bug existed then the separate task of adding new WebKit 2 API wouldn't be necessary then we should close 76063 as Invalid and just fix this bug! > > Sorry I meant to say: > If you're saying that if this bug DID NOT EXIST then the separate task of adding new WebKit 2 API wouldn't be necessary, then we should close 76063 as Invalid and just fix this bug! I did not mean to offend. My point was only that if this bug had not already filed, I wouldn't bother to file it separately, but it is a hypothetical question at this point so let us forget it and stick to the code :)
(In reply to comment #14) > (In reply to comment #11) >My point was only that if this bug had not already filed, I wouldn't bother to file it separately, but it is a hypothetical question at this point so let us forget it and stick to the code :) Especially since we have code ready to be a layout test! Excited to see your patch.
Created attachment 123706 [details] Patch
(In reply to comment #15) > (In reply to comment #14) > > (In reply to comment #11) > >My point was only that if this bug had not already filed, I wouldn't bother to file it separately, but it is a hypothetical question at this point so let us forget it and stick to the code :) > > Especially since we have code ready to be a layout test! Excited to see your patch. I am not sure I can make that into a layout test. I will check, but it may at least be a manual test.
After some investigation, I think my patch is only a general solution to https://bugs.webkit.org/show_bug.cgi?id=53648 so that the API doesn't depend on the order of suspending and triggering onpagehide events. This particular bug, or at least the attached test-case is not caused by inconsistency of suspending active dom-objects, but on timeouts being installed on the mainframe. In the test-case the timeout from a-frame is installed on the main frame, and correctly suspended, but the main frame is then reused for the new page where all active dom objects are resumed, including the timeouts of subframes of the old page. At least that is how it looks to me currently. I will unlink the bugs, as it was not as related as it looked at first.
(In reply to comment #17) > (In reply to comment #15) > > (In reply to comment #14) > > > (In reply to comment #11) > > >My point was only that if this bug had not already filed, I wouldn't bother to file it separately, but it is a hypothetical question at this point so let us forget it and stick to the code :) > > > > Especially since we have code ready to be a layout test! Excited to see your patch. > > I am not sure I can make that into a layout test. I will check, but it may at least be a manual test. I don't see why it can't. Can you say why you're having a hard time making it in to one? (In reply to comment #18) > After some investigation, I think my patch is only a general solution to https://bugs.webkit.org/show_bug.cgi?id=53648 so that the API doesn't depend on the order of suspending and triggering onpagehide events. > > This particular bug, or at least the attached test-case is not caused by inconsistency of suspending active dom-objects, but on timeouts being installed on the mainframe. Indeed the test case is about a timeout being added on the mainframe. It would manifest in almost the exact same way by adding an ActiveDOMObject during pagehide. To support the API you wish to add you'll have to solve the problem with *all* elements on a page that need to be suspended. That includes ActiveDOMObjects *and* timers, as well as any others that might be floating around out there. > In the test-case the timeout from a-frame is installed on the main frame, and correctly suspended, but the main frame is then reused for the new page where all active dom objects are resumed, including the timeouts of subframes of the old page. > > At least that is how it looks to me currently. I will unlink the bugs, as it was not as related as it looked at first. Why does it look that way? That would be surprising to me.
(In reply to comment #19) > > This particular bug, or at least the attached test-case is not caused by inconsistency of suspending active dom-objects, but on timeouts being installed on the mainframe. Actually, timeouts are implemented by the SuspendableTimer class, which subclasses ActiveDOMObject.
(In reply to comment #20) > (In reply to comment #19) > > > This particular bug, or at least the attached test-case is not caused by inconsistency of suspending active dom-objects, but on timeouts being installed on the mainframe. > > Actually, timeouts are implemented by the SuspendableTimer class, which subclasses ActiveDOMObject. More accurately, DOMWindow::setTimeout creates a DOMTimer, which subclasses SuspendableTimer, which in turn sublcasses ActiveDOMObject.
(In reply to comment #19) > (In reply to comment #17) > > I am not sure I can make that into a layout test. I will check, but it may at least be a manual test. > > I don't see why it can't. Can you say why you're having a hard time making it in to one? > I didn't. I had just missed one of the comments here. It was pretty simple with that information. > (In reply to comment #18) > > Why does it look that way? That would be surprising to me. What I mean is that my current patch did not touch the code this bug is in, as I thought it did. It will still need to be solved, but I am decoupling the bugs as two separate task like you originally asked for.
(In reply to comment #21) > (In reply to comment #20) > > (In reply to comment #19) > > > > This particular bug, or at least the attached test-case is not caused by inconsistency of suspending active dom-objects, but on timeouts being installed on the mainframe. > > > > Actually, timeouts are implemented by the SuspendableTimer class, which subclasses ActiveDOMObject. > > More accurately, DOMWindow::setTimeout creates a DOMTimer, which subclasses SuspendableTimer, which in turn sublcasses ActiveDOMObject. I know. The problem is that they remain in the same ScriptExecutionContext, so they are suspended as ActiveDOMObjects, but they are also resumed as ActiveDOMObjects because the main frame is reused for the new page. So the problem seems to be that CachedFrame doesn't store these DOMTimers somewhere before reusing the Frame.
(In reply to comment #23) > (In reply to comment #21) > > (In reply to comment #20) > > > (In reply to comment #19) > > > > > This particular bug, or at least the attached test-case is not caused by inconsistency of suspending active dom-objects, but on timeouts being installed on the mainframe. > > > > > > Actually, timeouts are implemented by the SuspendableTimer class, which subclasses ActiveDOMObject. > > > > More accurately, DOMWindow::setTimeout creates a DOMTimer, which subclasses SuspendableTimer, which in turn sublcasses ActiveDOMObject. > > I know. The problem is that they remain in the same ScriptExecutionContext, so they are suspended as ActiveDOMObjects, but they are also resumed as ActiveDOMObjects because the main frame is reused for the new page. The main frame is reused, yes. But the ScriptExecutionContext is the Document, and when you navigate you get a new Document in the frame. > So the problem seems to be that CachedFrame doesn't store these DOMTimers somewhere before reusing the Frame. CachedFrame stores off the Document, which is the ScriptExecutionContext that holds those timers.
(In reply to comment #24) > (In reply to comment #23) > > I know. The problem is that they remain in the same ScriptExecutionContext, so they are suspended as ActiveDOMObjects, but they are also resumed as ActiveDOMObjects because the main frame is reused for the new page. > > The main frame is reused, yes. > > But the ScriptExecutionContext is the Document, and when you navigate you get a new Document in the frame. > > > So the problem seems to be that CachedFrame doesn't store these DOMTimers somewhere before reusing the Frame. > > CachedFrame stores off the Document, which is the ScriptExecutionContext that holds those timers. Then I misinterpreted what I saw. I inspected the timer to see what happened to it when the new page was loaded, and I could see it was suspended with SuspendableTimer::suspend() called from ScriptExecutionContext::suspendActiveDOMObjects() when the page was hidden, and was resumed with a SuspendableTimer::resume() called from ScriptExecutionContext::resumeActiveDOMObjects() after the new page was loaded and immediately before the alert was shown. I haven't yet investigated what calls resumeActiveDOMObjects() on a document in PageCache. I guess this will be the next step. I only concluded it was correctly suspended in the first place.
Created attachment 123937 [details] Patch
My analysis yesterday was based on faulty output. I traced the odd output I was seeing to virtual functions called from constructors. The method to maintain the suspend-state has now been moved to a post-constructor method, and is called from the static create functions for active DOM objects.
Attachment 123937 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/fileapi/FileReader.cpp:54: Declaration has space between type name and * in FileReader *fileReader [whitespace/declaration] [3] Source/WebCore/fileapi/FileWriter.cpp:51: Declaration has space between type name and * in FileWriter *fileWriter [whitespace/declaration] [3] Source/WebCore/websockets/WebSocket.cpp:169: Declaration has space between type name and * in WebSocket *webSocket [whitespace/declaration] [3] Source/WebCore/websockets/WebSocket.h:56: The parameter name "context" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/fileapi/FileWriter.h:51: The parameter name "context" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 5 in 22 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 123942 [details] Patch Fixed coding style
Comment on attachment 123942 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123942&action=review Can didCreateActiveDOMObject be merged with didInstallActiveDOMObject? This change makes the interface bigger and even more confusing, relying on manual work that can be easily forgotten. > Source/WebCore/ChangeLog:38 > + (FileReader): > + (): prepare-ChangeLog creates this list so that patch authors could put useful information here. Script mistakes need to be corrected manually. > Source/WebCore/fileapi/FileReader.cpp:51 > +// static We don't normally add such comments.
(In reply to comment #30) > (From update of attachment 123942 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=123942&action=review > > Can didCreateActiveDOMObject be merged with didInstallActiveDOMObject? > > This change makes the interface bigger and even more confusing, relying on manual work that can be easily forgotten. > This was what I started with, but I realized that didCreateActiveDOMObject is called from the ActiveDOMObject constructor and didInstallActiveDOMObject calls suspend() which is a virtual function. Calling virtual functions from a constructor does not work, but instead just does the function call as if it was not virtual (I wished they would be crash, because they create such obscure bugs). So a manual function call after the constructor is unfortunately required for any ActiveDOMObjects that implements suspend. If we could rely on C++11, it would be possible to make a inherited constructor, but for C++ in general and for a cross platform project like this I do not think it is possible.
Comment on attachment 123942 [details] Patch Attachment 123942 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11344604 New failing tests: fast/events/suspend-timers.html
Good point. Can we get rid of didCreate then?
(In reply to comment #33) > Good point. Can we get rid of didCreate then? I gave that some serious though, but the didCreateActiveDOMObject matches the didCreateDestructionObserver, and forms a set together with the willDestroy.. functions, all called from constructors and destructors. I do not want to break up a perfect pattern of matching similar functions. Perhaps a better design were the all registration functions were called after the constructors would be better, but redesigning all these functions is beyond this bug-fix.
I don't think that destruction observer should have any impact on ActiveDOMObject design.
(In reply to comment #35) > I don't think that destruction observer should have any impact on ActiveDOMObject design. All ActiveDOMObjects are also DestructionObservers (they even defined in the same header file ActiveDOMObject.h). I feel it is more logical if the functions that operate on them behave in the same way. didCreateActiveDOMObject - called from the constructor, and can be relied on having been called in inherited constructors didCreateDestructionObserver - called from the constructor, and can be relied on having been called in inherited constructors willDestroyActiveDOMObject - called from the destructor willDestroyDestructionObserver - called from the destructor didInstallActiveDOMObjects - called after the constructor, and can not be relied on having been called in inherited constructors, but can use virtual functions. If I merged didInstall and didCreate, the functions would no longer operate similar to destruction observers, and I would have to check all functions called from constructors of inherited ActiveDOMObjects to see if the rely on ActiveDOMObjects already being registered in the ScriptExecutionContext (it would be a classic ASSERT for instance). I do think there are way to many of these setup functions, and is not even sure why active DOM objects and destruction observers are accounted separately, but someone has designed it this way, and currently it has a very symmetric and predicable definition that I would hate to change in a bug-fix.
As an alternate fix to this bug (which couldn't involve any changes to ActiveDOMObject or its subclasses), did you consider my initial comment: "separate the recursion steps; one full pass to fire pagehide and another to suspend active DOM objects."? It might be as easy as moving lines 167 to 175 of CachedFrame.cpp to a separate function that is called after the CachedFrame constructor. Something like: PassRefPtr<CachedFrame> CachedFrame::create(Frame* frame) { // Will fire pagehide event handlers. RefPtr<CachedFrame> cachedFrame = adoptRef(new CachedFrame(frame)); // Only after we've stopped executing script can we suspend active DOM in the frame tree. cachedFrame->suspend(); return cachedFrame->release(); } CachedFrame::suspend() { m_document->documentWillSuspendForPageCache(); m_document->suspendScriptedAnimationControllerCallbacks(); m_document->suspendActiveDOMObjects(ActiveDOMObject::DocumentWillBecomeInactive); m_cachedFrameScriptData = adoptPtr(new ScriptCachedFrameData(frame)); for (unsigned i = 0; i < m_childFrames.size(); ++i) m_childFrames[i]->suspend(); }
(In reply to comment #37) > As an alternate fix to this bug (which couldn't involve any changes to ActiveDOMObject or its subclasses), did you consider my initial comment: "separate the recursion steps; one full pass to fire pagehide and another to suspend active DOM objects."? It might be as easy as moving lines 167 to 175 of CachedFrame.cpp to a separate function that is called after the CachedFrame That would solve the bug for the case of suspending when moving to pageCache, but It would not solve the bug generally. They way I discovered the bug, was from the new pagePaused API, which had the same problem of timers firing after they were supposed to be suspended. I need a general solution, not a work-around in CachedFrame.
(In reply to comment #38) > (In reply to comment #37) > > As an alternate fix to this bug (which couldn't involve any changes to ActiveDOMObject or its subclasses), did you consider my initial comment: "separate the recursion steps; one full pass to fire pagehide and another to suspend active DOM objects."? It might be as easy as moving lines 167 to 175 of CachedFrame.cpp to a separate function that is called after the CachedFrame > > That would solve the bug for the case of suspending when moving to pageCache, but It would not solve the bug generally. They way I discovered the bug, was from the new pagePaused API, which had the same problem of timers firing after they were supposed to be suspended. I need a general solution, not a work-around in CachedFrame. Indeed. In general we have a few places lurking in WebCore code where we try to put some object into some state, and by doing so we run some javascript, and that javascript invalidates what we're trying to do. I like that we're close to identifying a way to *actually* solve the problem at least in this one case, and I think we shouldn't stop short of doing so.
Created attachment 124286 [details] Patch Updated patch. Fixed comments. Call didInstallActiveDOMObjects for AbstractWorker subclasses as well, even though they don't use the suspend API, this makes the didInstallActiveDOMObjects universally called from all descendants of ActiveDOMObjects. Moved expected result to qt only, because I do not have the ability to test all platforms.
Created attachment 124288 [details] Patch Quick update: Moved expected result back to main, and just set Chromium to skip it. Hopefully all the other required platforms will have support for PageCache.
Created attachment 124289 [details] Patch Remember to update ChangeLog after moving file.
Comment on attachment 124289 [details] Patch Maybe it is better to call didInstallActiveDOMObject in the ctor of ActiveDOMObejct? Anyway, just FYI, database callbacks are probably also ActiveDOMObjects. It would be nice if we could guarantee no JS will be executed once we stop all active dom objects on the page. Maybe we can dispatch the hide event to the page before caching it?
(In reply to comment #43) > (From update of attachment 124289 [details]) > Maybe it is better to call didInstallActiveDOMObject in the ctor of ActiveDOMObejct? Anyway, just FYI, database callbacks are probably also ActiveDOMObjects. It would be nice if we could guarantee no JS will be executed once we stop all active dom objects on the page. Maybe we can dispatch the hide event to the page before caching it? As already mentioned in both the ChangeLog and the discussion about didInstallActiveDOMObject can not be called from the ctor of ActiveDOMObject because it may need to call the virtual function suspend(). I think you are right about database objects, I thought I did a full search, but I seem to have missed a few classes. I will try again, and upload a new patch once I am sure I have caught all Active DOM Object derivatives.
Created attachment 124529 [details] Patch Update of creation hooks after deep-search for all classes inherited by ActiveDOMObject.
Comment on attachment 124529 [details] Patch Attachment 124529 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11360889
Created attachment 124537 [details] Patch Avoid local name overload.
Comment on attachment 124537 [details] Patch Attachment 124537 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11366581
Created attachment 124548 [details] Patch Compiled and regression tested with both Qt and Chromium platforms.
Comment on attachment 124548 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124548&action=review Should this work be done by the base class? Having all the subclasses call didInstallActiveDOMObject seems error-prone. > Source/WebCore/Modules/intents/IntentRequest.cpp:44 > + IntentRequest* intentRequest = new IntentRequest(context, intent, successCallback, errorCallback); Please call adoptRef right away. You can then use release() at in the return statement.
Comment on attachment 124548 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124548&action=review I really don't like the need to have every subclass of ActiveDOMObject call didInstallActiveDOMObject. That's not going to scale well. > Source/WebCore/dom/ScriptExecutionContext.cpp:266 > +void ScriptExecutionContext::didInstallActiveDOMObject(ActiveDOMObject* object) > +{ > + ASSERT(m_activeDOMObjects.contains(object)); > + // Ensure all ActiveDOMObjects are suspended also newly created ones. > + if (m_activeDOMObjectsAreSuspended) > + object->suspend(m_suspendReason); > +} Why can't this work be done in didCreateActiveDOMObject ?
Comment on attachment 124548 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124548&action=review >> Source/WebCore/dom/ScriptExecutionContext.cpp:266 >> +} > > Why can't this work be done in didCreateActiveDOMObject ? didInstallActiveDOMObject may call suspend(), which is a virtual method. didCreateActiveDOMObject is called by the ActiveDOMObject (i.e. base class) constructor, so it won't get the right implementation of suspend(). An alternative implementation is to move the didCreateActiveDOMObject call from the ActiveDOMObject's constructor to each of the subclasses' constructors. It's still sucky that it has to be repeated everywhere, but at least it's more encapsulated (vs. having to remember to call it every time a instance is created).
Hum... Maybe we should move the didCreateActiveDOMObject call into the subclass constructor so that things won't work at all if you forget to call it. That might help future contributors not forget. In the approach used by this patch, forgetting the call leads to a subtle bug, not an obvious bug.
(In reply to comment #52) > (From update of attachment 124548 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=124548&action=review > > >> Source/WebCore/dom/ScriptExecutionContext.cpp:266 > >> +} > > > > Why can't this work be done in didCreateActiveDOMObject ? > > didInstallActiveDOMObject may call suspend(), which is a virtual method. didCreateActiveDOMObject is called by the ActiveDOMObject (i.e. base class) constructor, so it won't get the right implementation of suspend(). > > An alternative implementation is to move the didCreateActiveDOMObject call from the ActiveDOMObject's constructor to each of the subclasses' constructors. It's still sucky that it has to be repeated everywhere, but at least it's more encapsulated (vs. having to remember to call it every time a instance is created). Since all classes had the static 'create' function, it seemed safer to call it there. I was thinking about instead of calling didInstallActiveDOMObject, then making a virtual ActiveDOMObject::postConstructor and calling that from the static create functions. If something like this is needed elsewhere it could be a common pattern in WebKit. This way it is possible to have virtual post-constructors where-ever needed. I can think of many places this would be useful, and it could possible be integrated with a special version of adoptRef.
(In reply to comment #53) > Hum... Maybe we should move the didCreateActiveDOMObject call into the subclass constructor so that things won't work at all if you forget to call it. That might help future contributors not forget. In the approach used by this patch, forgetting the call leads to a subtle bug, not an obvious bug. Calling didCreateActiveDOMObject from subclass constructors could still lead to subtle bugs in later subclasses. For instance the abstract worker classes does not implement suspend, but inherit from ActiveDOMObject, a subclass could reimplement suspend, but if the abstract worker calls didCreateActiveDOMObject, the correct suspend function would never be called. This is why I prefer calling these functions after the object have been completely constructed.
The general postConstructor sounds a bit over designed. Let's start by solving the current problem. If this pattern recurs, we can generalize it in the future. Having the call in the create function sounds fine. Having things fail catastrophically rather than subtly is important. Perhaps we should also have a debug-only bool on ActiveDOMObject and an ASSERT in the ActiveDOMObject destructor to make sure that all subclasses drive this part of the lifecycle correctly.
(In reply to comment #44) > (In reply to comment #43) > > (From update of attachment 124289 [details] [details]) > > Maybe it is better to call didInstallActiveDOMObject in the ctor of ActiveDOMObejct? Anyway, just FYI, database callbacks are probably also ActiveDOMObjects. It would be nice if we could guarantee no JS will be executed once we stop all active dom objects on the page. Maybe we can dispatch the hide event to the page before caching it? > > As already mentioned in both the ChangeLog and the discussion about didInstallActiveDOMObject can not be called from the ctor of ActiveDOMObject because it may need to call the virtual function suspend(). Yeah, I realized that. But is there anything we can try like letting all derived classes know they are suspended at creation? Anyway, I just see the risk that we can miss some cases now or in the future. Forcing a change to ActiveDOMObject ctor can help find out all the derived classes.
(In reply to comment #56) > The general postConstructor sounds a bit over designed. Let's start by solving the current problem. If this pattern recurs, we can generalize it in the future. > > Having the call in the create function sounds fine. Having things fail catastrophically rather than subtly is important. Perhaps we should also have a debug-only bool on ActiveDOMObject and an ASSERT in the ActiveDOMObject destructor to make sure that all subclasses drive this part of the lifecycle correctly. Sounds like a good idea. It could be asserted several places in fact to even ensure fast crashes for developers. Unless there are any objections, I will make this addition tomorrow.
Created attachment 124706 [details] Patch Add special ASSERT flag to ActiveDOMObject to enforce correct behavior on debug builds. Rename didInstall.. to suspendIfNeeded so it describes what it actually does.
Attachment 124706 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9 Updating OpenSource First, rewinding head to replay your work on top of it... Applying: Fix compilation errors on build-webkit --debug --no-workers on mac. Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging LayoutTests/ChangeLog CONFLICT (content): Merge conflict in LayoutTests/ChangeLog Auto-merging LayoutTests/platform/qt/Skipped CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped Auto-merging Source/WebCore/ChangeLog CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog Failed to merge in the changes. Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac. When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 164. If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 124708 [details] Patch
Attachment 124708 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9 Updating OpenSource First, rewinding head to replay your work on top of it... Applying: Fix compilation errors on build-webkit --debug --no-workers on mac. Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging LayoutTests/ChangeLog CONFLICT (content): Merge conflict in LayoutTests/ChangeLog Auto-merging LayoutTests/platform/qt/Skipped CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped Auto-merging Source/WebCore/ChangeLog CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog Failed to merge in the changes. Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac. When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 164. If any of these errors are false positives, please file a bug against check-webkit-style.
See also: bug 77370.
Comment on attachment 124708 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124708&action=review Almost there. > Source/WebCore/dom/ActiveDOMObject.cpp:63 > + , m_suspendUptodate(false) The name of this is slightly awkward. Maybe call it m_suspendIfNeededCalled? > Source/WebCore/dom/ScriptExecutionContext.cpp:208 > + ASSERT(iter->first->scriptExecutionContext() == this && iter->first->m_suspendUptodate); Can you move the new check to its own ASSERT statement, so that that if it fails it's easier to see which one was responsible (applies below too)? Also, there should be a getter for the property (which should be private), since it should not be mutable by others. > Source/WebCore/page/SuspendableTimer.cpp:39 > +#if !ASSERT_DISABLED Why this change (making m_active be available for release builds)?
(In reply to comment #64) > (From update of attachment 124708 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=124708&action=review > > Almost there. > > > Source/WebCore/dom/ActiveDOMObject.cpp:63 > > + , m_suspendUptodate(false) > > The name of this is slightly awkward. Maybe call it m_suspendIfNeededCalled? > It was named from an ASSERT point-of-view. If suspendIfNeeded has been called, then we are sure suspend is fully updated, but I will rename it. > > Source/WebCore/dom/ScriptExecutionContext.cpp:208 > > + ASSERT(iter->first->scriptExecutionContext() == this && iter->first->m_suspendUptodate); > > Can you move the new check to its own ASSERT statement, so that that if it fails it's easier to see which one was responsible (applies below too)? > > Also, there should be a getter for the property (which should be private), since it should not be mutable by others. > Okay, I just didn't want to add an ASSERT-only function, which is why I made it public. > > Source/WebCore/page/SuspendableTimer.cpp:39 > > +#if !ASSERT_DISABLED > > Why this change (making m_active be available for release builds)? It only changes the constructor. m_active is already available on release builds, it just isn't initialized. It is not a real bug though since bools defaults to being initialized to false, but it is certainly a mistake, which is why I fixed it while debugging the class.
Created attachment 125100 [details] Patch
(In reply to comment #65) > ...It is not a real bug though since bools defaults to being initialized to false. Not true in C++, except static members.
Is this patch ready to land?
Comment on attachment 125100 [details] Patch Rejecting attachment 125100 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: om/ActiveDOMObject.h:58: error: expected ';' before '}' token make: *** [out/Debug/obj.target/webcore_bindings/gen/webkit/bindings/V8DerivedSources04.o] Error 1 make: *** Waiting for unfinished jobs.... make: *** [out/Debug/obj.target/webcore_bindings/gen/webkit/bindings/V8DerivedSources01.o] Error 1 make: *** [out/Debug/obj.target/webcore_bindings/gen/webkit/bindings/V8DerivedSources03.o] Error 1 make: *** [out/Debug/obj.target/webcore_bindings/gen/webkit/bindings/V8DerivedSources02.o] Error 1 Full output: http://queues.webkit.org/results/11386983
Created attachment 125297 [details] Patch I needed to ensure the assertions actually compiled and worked after the last rename.
Created attachment 125305 [details] Patch for landing
(In reply to comment #71) > Created an attachment (id=125305) [details] > Patch for landing Misplaced semicolon failed debug-build.
Comment on attachment 125305 [details] Patch for landing Clearing flags on attachment: 125305 Committed r106654: <http://trac.webkit.org/changeset/106654>
All reviewed patches have been landed. Closing bug.
Reopen, because it made many tests assert in debug mode: http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Debug/r106654%20%2820860%29/results.html
(In reply to comment #75) > Reopen, because it made many tests assert in debug mode: > http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Debug/r106654%20%2820860%29/results.html Rolled out in r106667.
Created attachment 125603 [details] Patch for landing In Worker::create and SharedWorker::Create move the call to suspendIfNeeded up before the first exit of the create function.
Comment on attachment 125603 [details] Patch for landing Clearing flags on attachment: 125603 Committed r106797: <http://trac.webkit.org/changeset/106797>
The change has caused some ASSERTs on ToT (see bug 77868 for example but I am seeing those errors also in webaudio). Allan, here is the link to the failing tests: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=webaudio%2Fmediaelementaudiosourcenode-gc.html%2Cwebaudio%2Fsample-accurate-scheduling.html%2Cwebaudio%2Fdelaynode-scheduling.html%2Cwebaudio%2Fdistance-exponential.html%2Cwebaudio%2Fdistance-linear.html%2Cwebaudio%2Fbiquad-highshelf.html%2Cwebaudio%2Fbiquad-peaking.html%2Cwebaudio%2Faudionode-connect-order.html%2Cwebaudio%2Fbiquad-lowshelf.html%2Cwebaudio%2Fnote-grain-on-play.html%2Cwebaudio%2Fpanner-equalpower.html%2Cwebaudio%2Fbiquad-bandpass.html%2Cwebaudio%2Faudiobuffersource.html I rolled out the patch and the follow up in <http://trac.webkit.org/changeset/106823>.
(In reply to comment #80) > The change has caused some ASSERTs on ToT (see bug 77868 for example but I am seeing those errors also in webaudio). Allan, here is the link to the failing tests: > > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=webaudio%2Fmediaelementaudiosourcenode-gc.html%2Cwebaudio%2Fsample-accurate-scheduling.html%2Cwebaudio%2Fdelaynode-scheduling.html%2Cwebaudio%2Fdistance-exponential.html%2Cwebaudio%2Fdistance-linear.html%2Cwebaudio%2Fbiquad-highshelf.html%2Cwebaudio%2Fbiquad-peaking.html%2Cwebaudio%2Faudionode-connect-order.html%2Cwebaudio%2Fbiquad-lowshelf.html%2Cwebaudio%2Fnote-grain-on-play.html%2Cwebaudio%2Fpanner-equalpower.html%2Cwebaudio%2Fbiquad-bandpass.html%2Cwebaudio%2Faudiobuffersource.html > > I rolled out the patch and the follow up in <http://trac.webkit.org/changeset/106823>. I have run all the enabled tests on Chromium on all of the committed patches, but there appears to be a load of tests tested by your bot that are not tested in a default run of testrunner. Is there a way I can get a list of parameters to give the testrunner to get all the tests that you expect to pass? Or possibly if you have a setup that runs all the tests, then make the bot run ALL of them so they can be fixed in one go?
The bot just runs the same set of tests that you'd run with run-webkit-tests --chromium. Maybe you tried a release build and that's why you didn't see the ASSERT?
Created attachment 125781 [details] Patch Call suspendIfNeeded() from the second AudioContext creator function.
(In reply to comment #82) > The bot just runs the same set of tests that you'd run with run-webkit-tests --chromium. Maybe you tried a release build and that's why you didn't see the ASSERT? No I run 'run-webkit-tests --chromium --debug'. The problem is that neither the tests in storage/indexeddb or webaudio are run by this default run. I have to run these two directories of tests manually afterwards. I can understand these features might be under development, it is just unfortunate that tests that are expected to work are not included in a default run of run-webkit-tests.
I followed the issues related to calling virtual methods in constructors, but the technique adopted by this patch is very fragile. Every time someone adds a new active DOM object without remember to call suspendIfNeeded() in the ctor, stuff is going to break again in hard-to-find ways. You really need some compile-time warning, or something that "just works" here.
(In reply to comment #85) > I followed the issues related to calling virtual methods in constructors, but the technique adopted by this patch is very fragile. Every time someone adds a new active DOM object without remember to call suspendIfNeeded() in the ctor, stuff is going to break again in hard-to-find ways. You really need some compile-time warning, or something that "just works" here. I am wondering, how this will affect the web components work, Dimitri?
(In reply to comment #85) > I followed the issues related to calling virtual methods in constructors, but the technique adopted by this patch is very fragile. Every time someone adds a new active DOM object without remember to call suspendIfNeeded() in the ctor, stuff is going to break again in hard-to-find ways. You really need some compile-time warning, or something that "just works" here. This is why we have the ASSERT. It is not hard to find. You just need to run any test that creates this kind of object and the ASSERT will catch it if you haven't called suspendIfNeeded. What is happening right now for instance is that all the cases not found by inspection is caught by this assert, so there is no way any hard-to-find bugs will pop up.
Comment on attachment 125781 [details] Patch Clearing flags on attachment: 125781 Committed r107239: <http://trac.webkit.org/changeset/107239>
Comment on attachment 125781 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125781&action=review > Source/WebCore/page/DOMTimer.cpp:95 > DOMTimer* timer = new DOMTimer(context, action, timeout, singleShot); > > + timer->suspendIfNeeded(); So DOMTimer::suspendIfNeeded() calls ScriptExecutionContext::suspendActiveDOMObjectIfNeeded, which uses m_reasonForSuspendingActiveDOMObjects. What guarantees that m_reasonForSuspendingActiveDOMObjects is correct here? It's really unclear to me what "IfNeeded" part means in function name. For example, if the object is already suspended, should suspendIfNeeded() be a no-op? Or should it change the reason? what happens in practice is that an assertion fails.
(In reply to comment #90) > (From update of attachment 125781 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=125781&action=review > > > Source/WebCore/page/DOMTimer.cpp:95 > > DOMTimer* timer = new DOMTimer(context, action, timeout, singleShot); > > > > + timer->suspendIfNeeded(); > > So DOMTimer::suspendIfNeeded() calls ScriptExecutionContext::suspendActiveDOMObjectIfNeeded, which uses m_reasonForSuspendingActiveDOMObjects. What guarantees that m_reasonForSuspendingActiveDOMObjects is correct here? > > It's really unclear to me what "IfNeeded" part means in function name. For example, if the object is already suspended, should suspendIfNeeded() be a no-op? Or should it change the reason? what happens in practice is that an assertion fails. It means the active object should be suspended if the ScriptExecutionContext is currently suspending all active objects. You can also call it initialize suspend state or suspend if suspending active DOM objects. I think this was as clear as it gets though.