Bug 141977 – Regression(r164091): Pages using URL.createObjectURL() cannot enter the PageCache

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Chris Dumez]

Make PublicURLManager suspendable. There is nothing preventing it from being suspended. It is used on baidu.com and prevents pages from going into the PageCache unnecessarily.

Radar: <rdar://problem/19923085>

[Chris Dumez]

This is a regression from <http://trac.webkit.org/changeset/164091>.

[Chris Dumez]

At first, I thought I would simply mark PublicURLManager as suspendable. However, I think the correct fix is for PublicURLManager to not be an ActiveDOMObject (because it never causes any JS to be executed).

The reference cycle from Bug 128532 can be likely fixed by clearing the URLRegistry from Document::prepareForDestruction() instead of from ~ScriptExecutionContext().

[Byungseon(Sun) Shin]

(In reply to comment #2)
> At first, I thought I would simply mark PublicURLManager as suspendable.
> However, I think the correct fix is for PublicURLManager to not be an
> ActiveDOMObject (because it never causes any JS to be executed).
> 
> The reference cycle from Bug 128532 can be likely fixed by clearing the
> URLRegistry from Document::prepareForDestruction() instead of from
> ~ScriptExecutionContext().

@Chris, thanks for sharing this issue.
I will check memory leakage issue by using "Document::prepareForDestruction()" and update here soon.

[Chris Dumez]

(In reply to comment #3)
> (In reply to comment #2)
> > At first, I thought I would simply mark PublicURLManager as suspendable.
> > However, I think the correct fix is for PublicURLManager to not be an
> > ActiveDOMObject (because it never causes any JS to be executed).
> > 
> > The reference cycle from Bug 128532 can be likely fixed by clearing the
> > URLRegistry from Document::prepareForDestruction() instead of from
> > ~ScriptExecutionContext().
> 
> @Chris, thanks for sharing this issue.
> I will check memory leakage issue by using
> "Document::prepareForDestruction()" and update here soon.

I am actually working on it already.

[Chris Dumez]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > At first, I thought I would simply mark PublicURLManager as suspendable.
> > > However, I think the correct fix is for PublicURLManager to not be an
> > > ActiveDOMObject (because it never causes any JS to be executed).
> > > 
> > > The reference cycle from Bug 128532 can be likely fixed by clearing the
> > > URLRegistry from Document::prepareForDestruction() instead of from
> > > ~ScriptExecutionContext().
> > 
> > @Chris, thanks for sharing this issue.
> > I will check memory leakage issue by using
> > "Document::prepareForDestruction()" and update here soon.
> 
> I am actually working on it already.

The reason I haven't uploaded a patch is because I am trying to figure out why MediaSource would ref the Document in the first place.

[Byungseon(Sun) Shin]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (In reply to comment #2)
> > > > At first, I thought I would simply mark PublicURLManager as suspendable.
> > > > However, I think the correct fix is for PublicURLManager to not be an
> > > > ActiveDOMObject (because it never causes any JS to be executed).
> > > > 
> > > > The reference cycle from Bug 128532 can be likely fixed by clearing the
> > > > URLRegistry from Document::prepareForDestruction() instead of from
> > > > ~ScriptExecutionContext().
> > > 
> > > @Chris, thanks for sharing this issue.
> > > I will check memory leakage issue by using
> > > "Document::prepareForDestruction()" and update here soon.
> > 
> > I am actually working on it already.
> 
> The reason I haven't uploaded a patch is because I am trying to figure out
> why MediaSource would ref the Document in the first place.

I see. Thanks for the reply.

[Chris Dumez]

Created attachment 247310 [details]
Patch

[Andreas Kling]

Comment on attachment 247310 [details]
Patch

I suppose this is reasonable.

[WebKit Commit Bot]

Comment on attachment 247310 [details]
Patch

Clearing flags on attachment: 247310

Committed r180646: <http://trac.webkit.org/changeset/180646>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Brent Fulgham]

Did you check if eventSender.beginDragWithFiles(). is supported on Windows? The test bots say "NO".  :-|

[Brent Fulgham]

Windows marked as Skip in <https://trac.webkit.org/changeset/180650>