Bug 61326 – fast/storage/storage-detached-iframe.html is crashing on chromium-linux

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Julien Chaffraix]

Follow up on bug 57140. I tried to find the cause of this problem but the backtrace does not help (see the previous bug for the EWS one).

I suspect that it is an issue unrelated to the change that is making it fail as other platforms work fine (Qt/Linux + Mac).

[Justin Schuh]

Crashing on all Chromium platforms.

Created attachment 120881 [details]
Patch

Darin, can you review please?

Is my assumption correct that detached iframes don't have a webView?

[Darin Fisher (:fishd, Google)]

Comment on attachment 120881 [details]
Patch

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

> Source/WebKit/chromium/src/StorageAreaProxy.cpp:169
>  {

it might be more canonical to add an early check to see if frame->page() is null, and then return if so.

Created attachment 120978 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
From git://git.webkit.org/WebKit
   8e973cb..e1e2538  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 103955 = 6aab0dafec68b43a47f81ab79d03001e05e99b64
r103949 = e1e2538cff7d320c2b7cc22c0b75c9369ce9cdb2
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
	A	LayoutTests/svg/custom/webkit-transform-crash.html
	A	LayoutTests/svg/custom/webkit-transform-crash-expected.txt
	M	LayoutTests/ChangeLog
	M	Source/WebCore/ChangeLog
	M	Source/WebCore/svg/SVGStyledTransformableElement.cpp
103950 = 8282a1d85ad097ce4064a5896912bdb211b115e6 already exists! Why are we refetching it?
 at /usr/lib/git-core/git-svn line 5210

Died at Tools/Scripts/update-webkit line 158.


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

[WebKit Review Bot]

Comment on attachment 120978 [details]
Patch

Attachment 120978 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11084081

Created attachment 120979 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
Index mismatch: 3a7674dadd24ecd6a1b023eba203ecf2ed62fc59 != edb861612940a3244431d239dacdbc36317b627c
rereading e1e2538cff7d320c2b7cc22c0b75c9369ce9cdb2
	A	LayoutTests/svg/custom/webkit-transform-crash.html
	A	LayoutTests/svg/custom/webkit-transform-crash-expected.txt
	M	LayoutTests/ChangeLog
	M	Source/WebCore/ChangeLog
	M	Source/WebCore/svg/SVGStyledTransformableElement.cpp
103950 = 8282a1d85ad097ce4064a5896912bdb211b115e6 already exists! Why are we refetching it?
 at /usr/lib/git-core/git-svn line 5210

Died at Tools/Scripts/update-webkit line 158.


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

PTAL

The style bot error is because of a problem with the bots checkout, unrelated to my CL

[Adam Barth]

Comment on attachment 120979 [details]
Patch

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

> Source/WebKit/chromium/src/StorageAreaProxy.cpp:171
> +      return false;

Bad indent.  I don't see where frame->page() is accessed in this function.  Would it be better to null check something else?  For example, maybe we should null-check the webView.

> LayoutTests/fast/storage/storage-detached-iframe.html:26
> +          } catch (e) {
> +            // Throws an exception on chromium, and fails silently on other platforms.
> +          }

Should we make Chromium and other ports work the same?

(In reply to comment #11)
> (From update of attachment 120979 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120979&action=review
> 
> > Source/WebKit/chromium/src/StorageAreaProxy.cpp:171
> > +      return false;
> 
> Bad indent.  I don't see where frame->page() is accessed in this function.  Would it be better to null check something else?  For example, maybe we should null-check the webView.

See Darin's comment.

Basically, the fact that frame->page() is NULL is canonical for the frame being detached.


> 
> > LayoutTests/fast/storage/storage-detached-iframe.html:26
> > +          } catch (e) {
> > +            // Throws an exception on chromium, and fails silently on other platforms.
> > +          }
> 
> Should we make Chromium and other ports work the same?

Actually, other ports don't fail silently, but the access works for them :-/

We can't just allow access if there's no page. A possible work around would be to move permissionClient() from WebView to WebFrameClient, wdyt?

[Adam Barth]

(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 120979 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=120979&action=review
> > 
> > > Source/WebKit/chromium/src/StorageAreaProxy.cpp:171
> > > +      return false;
> > 
> > Bad indent.  I don't see where frame->page() is accessed in this function.  Would it be better to null check something else?  For example, maybe we should null-check the webView.
> 
> See Darin's comment.
> 
> Basically, the fact that frame->page() is NULL is canonical for the frame being detached.

Ok. 

> >  > LayoutTests/fast/storage/storage-detached-iframe.html:26
> > > +          } catch (e) {
> > > +            // Throws an exception on chromium, and fails silently on other platforms.
> > > +          }
> > 
> > Should we make Chromium and other ports work the same?
> 
> Actually, other ports don't fail silently, but the access works for them :-/
> 
> We can't just allow access if there's no page. A possible work around would be to move permissionClient() from WebView to WebFrameClient, wdyt?

Hum...  How do other browsers handle this case?  We probably don't want any ports to allow access to local storage while detached.

Created attachment 121334 [details]
Patch

[Adam Barth]

Comment on attachment 121334 [details]
Patch

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

> Source/WebKit/chromium/src/StorageAreaProxy.cpp:170
> +    // Disallow all access to storage on detached frames.

I would skip these comments.  They're redundant with the code.

> LayoutTests/fast/storage/storage-detached-iframe.html:24
> +          } catch (e) {

Should we print a success message in this catch?

Created attachment 121346 [details]
Patch

[WebKit Review Bot]

Comment on attachment 121346 [details]
Patch

Clearing flags on attachment: 121346

Committed r104257: <http://trac.webkit.org/changeset/104257>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.