Bug 18941 – WebView fails to load if opened from a blocking JS call in a timer.

• Status: RESOLVED
• Resolution: WONTFIX
• Component: New Bugs

[Jeremy Moskovich]

Gears uses HTML for it's UI (a few simple dialogs).  I display these dialogs in Safari by opening a modal webview on top of the current Safari window.

This works fine on Leopard, but on Tiger I'm running into some weird problems where the contents of the modal WebView won't load if it's displayed while the calling Safari window is still loading it's contents.

I've attached a minimal testcase in the form of a hacked version of Apple's sample NPAPI plugin code.

Steps to reproduce:
* Unzip attached file and build.
* Run (a script in the run phase will automatically install the plugin), Safari will launch.
* Open modal_webview_ok.html file in Safari and click the "Trigger Modal WebView" button, notice that the sheet loads fine and displays Apple's website after a couple of seconds both on Tiger and Leopard.
* You will need to kill Safari in between these (sorry, didn't provide functionality to close the sheet0.
* Open modal_webview_tiger_bug.html - on Tiger the sheet is displayed but it's contents aren't loaded, works fine on Leopard.

We think this may be a WebKit bug as we've been having similar problems with WebKit on Android.

[Jeremy Moskovich]

Created attachment 21019 [details]
Minimal test case

[Jeremy Moskovich]

Created attachment 21647 [details]
Further reduction.

[Jeremy Moskovich]

Reduced this bug further with the help of Timothy:

Displaying a webview that blocks JS from a timer causes that new WebView to never be drawn.

Updated Minimal testcase:
* Creates a non-modal WebView in a window that has it's content set via WebFrame's loadHTMLString:baseURL: (should be no network traffic involved).
* Freeze occurs when making the blocking call from inside a timer.

Steps to reproduce:
* Unzip attached file and build.
* Run (a script in the run phase will automatically install the plugin), Safari
will launch.
* Open modal_webview.html in browser (ignore name of html file, the webview that's opened is non-modal).
* Click the "This displays OK" button - new WebView opens in a window and displays content.
* Click the second button - notice window opened but nothing is displayed.

If you add an NSButton to the window, it's rendered correctly so this appears to be a WebKit problem.

[Jeremy Moskovich]

Created attachment 21649 [details]
Further reduction

[Jeremy Moskovich]

This happens on both Leopard and Tiger.

[Timothy Hatcher]

Created attachment 21681 [details]
Proposed fix for comments

This patch fixes the attached test case. Basically the layout and loading related Timers were not firing in the nested event loop that the plugin was spinning up to block the calling JavaScript. I added the TimerBase::fireTimersInNestedEventLoop() call where we are calling native code for all the bindings (like we do with JSLock::DropAllLocks).

Is this the right fix?

Should we call TimerBase::fireTimersInNestedEventLoop() for all/some delegate methods too?

[Darin Adler]

Comment on attachment 21681 [details]
Proposed fix for comments

I'm not sure this is right. Someone (Mike Emmel, I think) was trying to get me to entirely remove the fireTimersInNestedEventLoop function and rework timers. Even if we do need calls to that function, I don't think the bindings are the right level to make the call.

[Dave Hyatt]

We need to make sure it's not legal for another JS timer to fire from inside the callback of an earlier JS timer.  That should never be allowed to happen.

[Jeremy Moskovich]

Calling TimerBase::fireTimersInNestedEventLoop() fixes this issue, however it also has the effect of enabling timers for the main Window which continue to fire behind the modal window.

Any ideas on how I can block JS timers for the main window while the dialog is displayed?

Another thing I should mention is that this bug is also present for showModalDialog() on 10.5 i.e. calling showModalDialog() from inside a Timer displays a blank window.

[Jeremy Moskovich]

Created attachment 23576 [details]
Adds mask to fix recursive JS timer invocations in previous patch.

The previous patch had the problem that once fireTimersInNestedEventLoops() had been called, JS timers where enabled along with all other timers causing a situation in which JS timers could fire recursively.

Adding code to fireTimersInNestedEventLoops() to mask certain types of timers isn't feasible because you have no way of knowing when to remove the mask.

This patch tags each timer with a type (currently just DOMTimers & generic timers for everything else) and allows a timer to signify exclusion of other timer types while it's firing.

If we encounter a fire that's excluded by the mask then we defer it to fire at a later time.

This means that you can call fireTimersInNestedEventLoop() at any point without worrying about the recursive behavior.

[Dave Hyatt]

Please drop the "k" from your enum constants.  We don't preface enum constants in WebKit.

[Jeremy Moskovich]

Created attachment 23580 [details]
Fix enum naming

Fixed enum naming to more closely match WebKit style.

[Eric Seidel (no email)]

This change looks fine to me.  But I'm certainly not the expert on WebCore timers.  Darin is probably the best person to review this.

[Alp Toker]

(In reply to comment #7)
> (From update of attachment 21681 [details] [edit])
> I'm not sure this is right. Someone (Mike Emmel, I think) was trying to get me
> to entirely remove the fireTimersInNestedEventLoop function and rework timers.
> Even if we do need calls to that function, I don't think the bindings are the
> right level to make the call.
> 

Mike's patch is in bug #18044

The idea here is to remove the special cases for nested event loop polling and cooperate better with the main loop. I like the idea but haven't got round to testing it.

[Darin Adler]

Comment on attachment 23580 [details]
Fix enum naming

I'm glad you're tackling this problem.

This patch changes timers so they can reenter. It's great that it preserves the property that DOM timers don't reenter. But it allows all other timers to get called in new way. I expect that will cause problems. Did you audit any of the 42 timers used in WebCore to check if it is OK to allow them all to reenter? What kind of testing did you do?

The language bridging layer is not a good place to call fireTimersInNestedEventLoop. I suppose the justification is that it's a call "out" of WebKit, but it's a blurring of responsibilities and a layering violation for the bindings mechanism to know about timers at all. The bindings are a language-bridging mechanism, and the event loop and timer machinery should not be involved. Further, there is a more modern JavaScript API for calls to and from JavaScript that doesn't involve this old bridging code. If this is necessary we'd need to find a way to do it for that mechanism too.

I think the solution to this design quandry is that we should eliminate the fireTimersInNestedEventLoop function entirely if we can make the disabling mechanism work well enough to prevent reentrancy.

+// Duplicated from JSDOMWindowBase.cpp - what would be a good common place to put this? 
+static const double cMinimumTimerInterval = 0.010;

This is wrong. This minimum timer interval is not a feature of the Timer class. It's a feature specifically of timers created from JavaScript with the DOM window API and should have no affect on other timers. It is incorrect to put code knowing about this minimum into Timer.cpp.

+        // Check if this timer is disabled by the current mask (e.g. if we're being called from inside a 
+        // JS Timer and this is also a JS Timer).  If so, delay execution till later.
+        unsigned mask_result = timer->type() & WebCore::timerMask;
+        if (mask_result == 0) {
+            timer->setNextFireTime(fireTime + cMinimumTimerInterval);
+            continue;
+        }

It's not a good design to change the firing time if a timer doesn't qualify for firing right away. Consider that if the timer mask changes, we'd want to fire the timer right away. And if the timer mask doesn't change, it's important to not to have the presence of a disabled timer cause the system timer to fire over and over again.

Instead the algorithm and data structure should skip over the timers that don't qualify rather than removing and re-adding them to the timer heap each time they fire erroneously.

I think a workable design would be to have a separate timer heap for each timer type. I don't think we're going to need tons of different timer types -- the theoretical generality of the current patch isn't all that important if we have only two types of timer.

+        unsigned savedTimerMask = WebCore::timerMask;
+        WebCore::timerMask = timer->timerMask();

I believe the timer disabling should be cumulative. So if you have certain timers disabled, firing a timer shouldn't enable them. Therefore, this should be doing an &= rather than an assignment.

----------------

Other more-minor code style comments:

+    WebCore::TimerBase::fireTimersInNestedEventLoop();

The normal way to do this would be to put "using namespace WebCore" at the top of the file rather than qualifying every use of TimerBase with the namespace.

 #import "FoundationExtras.h"
+#include "Timer.h"
 #import "WebScriptObject.h"

If the others are #import, then the Timer.h one should be too.

+    // Types to identify categories of timers.
+    typedef enum {
+        TimerTypeDOMWindowTimer = 1 << 0,
+        TimerTypeGenericTimer   = 1 << 1,
+        TimerTypeAllTimers      = 0xFFFF
+    } TimerType;

This should just be "enum TimerType"; there's no need for typedef here in C++.

-    : m_nextFireTime(0), m_repeatInterval(0), m_heapIndex(-1)
+    : m_nextFireTime(0), m_repeatInterval(0), m_heapIndex(-1), m_type(TimerTypeGenericTimer)
+    , m_timerMask(TimerTypeAllTimers)

Our usual style is to either put everything on one line, or everything on a separate line. So since you're overflowing one line you should put these all on separate lines.

[Jeremy Moskovich]

Thanks for the detailed feedback.

I believe there are actually two orthogonal issues here:
1. Come to a decision regarding a mechansim to enable timers in nested event loops.
2. Making sure JS timers aren't reentrant.

Would it be OK with you to tackle these issues one by one?  It seems that solving the second issue would be easier in the short term and would apply
to whatever solution we have for the first.

Would the following changes to the patch be acceptable to you:
1. Remove the calls to fireTimersInNestedEventLoop()
2. As you suggest maintain 2 separate heaps for JS timers and other types of timers.

I can create a separate bug for "mechanism to prevent reentrancy in JS timers" if that would help separate the issues.

[Darin Adler]

(In reply to comment #16)
> Would it be OK with you to tackle these issues one by one?

Sure.

> 1. Remove the calls to fireTimersInNestedEventLoop()

Remove all calls to it? Or don't add new calls? I'm not sure I understand. I don't want to introduce a bug that doesn't already exist as an intermediate step in a plan that eventually fixes the bug some other way, but maybe that's not what's going on here.

> I can create a separate bug for "mechanism to prevent reentrancy in JS timers"
> if that would help separate the issues.

A separate bug might help. But given the title, I'm worried. As far as I know we don't have a reentrancy problem that needs to be fixed. So a bug saying we need to fix it seems a little strange.

[Ahmad Saleem]

From the attached patch, I can see some changes to "jni_objc.mm", which was related to Java plugin, which is not supported by Safari since version 12.

Further, the test case (Further Reduction) has <embed> as below for plugin to play video:

<embed name="moviePlugin" width="200" height="240" src="" type="video/x-netscapemovie"></embed>


NPAPI support is removed from Safari 14 onward and it is not supported in Webkit Builds like WebkitGTK as well. I think this can be marked as "RESOLVED WONTFIX". Thanks!

[Ryosuke Niwa]

Yup, won't fix.