Ticket #3661 (closed Bug: fixed)

Opened 5 years ago

Last modified 5 years ago

Safari 3.2 cache issue

Reported by: damo Owned by: martinkou
Priority: Normal Milestone: CKEditor 3.0
Component: General Version:
Keywords: IBM Confirmed Safari Review+ Cc:

Description

To reproduce:
Use Safari 3.2 on Windows. Ensure caching is not disabled.

  1. Open nightly Ajax example
  2. Click "Create Editor" (editor should open normally)
  3. Click "Close Editor" (editor should be closed normally)
  4. Repeat steps 2&3 to ensure the process is reproducible.
  5. On the same page, enter a new URL e.g. www.google.com and hit Enter.
  6. On the same page, enter the url back to the original Ajax example (e.g. http://nightly.ckeditor.com/3608/_samples/ajax.html) and hit Enter.
  7. Click "Create Editor"

At this point, the editor will not load until the page is reloaded. This error does not appear when cache is disabled in the browser

Attachments

3661.patch (850 bytes) - added by martinkou 5 years ago.
3661_debug.patch (1.9 KB) - added by martinkou 5 years ago.
cached.png (363.4 KB) - added by martinkou 5 years ago.
working.png (410.3 KB) - added by martinkou 5 years ago.

Change History

comment:1 Changed 5 years ago by martinkou

  • Owner set to martinkou
  • Status changed from new to assigned

comment:2 Changed 5 years ago by martinkou

  • Keywords Confirmed Safari added

Seems to me there's a race condition in scriptloader.js - still investigating...

comment:3 Changed 5 years ago by martinkou

  • Keywords Review? added

The bug is found to be caused by the timing of the onLoad function in scriptloader.js in Safari - Safari calls the function as soon as it is assigned to the <script> element when Safari finds the script is already cached.

Changed 5 years ago by martinkou

Changed 5 years ago by martinkou

Changed 5 years ago by martinkou

Changed 5 years ago by martinkou

comment:4 Changed 5 years ago by martinkou

I've talked to Garry a bit about this bug before lunch, but we were not too sure of the exact cause of this bug - all I got this morning was just a hypothesis, and thus the patch may be sub-optimal.

So to prove it's actually Safari calling onLoad too early, I've attached a debug patch which prints out debug messages. Also two screenshots - cached.png is when Safari is loading the scripts purely from cache and the editor didn't work; working.png is the usual case where the editor works.

So from the screenshots, when Safari is loading the scripts from cache, onLoad of a script tag would be called from inside or immediately after loadScript(). Since it is assumed onLoad shouldn't be called immediately in our logic, Safari's execution order is breaking the script loader's logic.

comment:5 Changed 5 years ago by martinkou

There's still one concern in the patch here...

Now that the setTimeout() is executed for every browser, it may negatively impact performance. But I'm not sure if it actually happens.

On the other hand, if I add an if statement to check for Safari, I'd have increased the core code size unnecessarily - the patch works in other browsers as it is.

So the current patch is choosing the smaller code size option.

comment:6 Changed 5 years ago by garry.yao

  • Keywords Review+ added; Review? removed

comment:7 Changed 5 years ago by martinkou

  • Status changed from assigned to closed
  • Resolution set to fixed

Fixed with [3633].

Click here for more info about our SVN system.

Note: See TracTickets for help on using tickets.
© 2003 – 2012 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy