Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2423213002:
Remove a bunch of pointless null checks in FrameLoaderClientImpl.
DescriptionRemove a bunch of unnecessary null checks in FrameLoaderClientImpl.
WebLocalFrame can no longer be created with a null client: it's
guaranteed to have a non-null client until it's detached. Since
FrameLoaderClient only becomes null when a LocalFrame is detached,
having a non-null FrameLoaderClient implies that WebFrameClient is
also non-null.
BUG=none
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : Remove unnecessary check #Patch Set 4 : Fix formatting. #
Total comments: 11
Messages
Total messages: 25 (17 generated)
The CQ bit was checked by dcheng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by dcheng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Remove a bunch of pointless null checks in FrameLoaderClientImpl. BUG= ========== to ========== Remove a bunch of unnecessary null checks in FrameLoaderClientImpl. WebLocalFrame can no longer be created with a null client: it's guaranteed to have a non-null client until it's detached. Since FrameLoaderClient only becomes null when a LocalFrame is detached, having a non-null FrameLoaderClient implies that WebFrameClient is also non-null. BUG=none ==========
dcheng@chromium.org changed reviewers: + esprehn@chromium.org, toyoshim@chromium.org
The CQ bit was checked by dcheng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by dcheng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
+esprehn for overall review +toyoshim for worker stuff (I don't really understand the shadow page and the logic behind which WebFrameClient methods are implemented) https://codereview.chromium.org/2423213002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.cpp (left): https://codereview.chromium.org/2423213002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.cpp:119: m_mainFrame->setClient(0); This breaks the invariant. For now, I've added early returns to the various WebFrameClient methods implemented by the workers. https://codereview.chromium.org/2423213002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.cpp:127: m_mainFrame->close(); The default WebFrameClient implementation already does this. https://codereview.chromium.org/2423213002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.cpp (right): https://codereview.chromium.org/2423213002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.cpp:356: WTF::unretained(this))); It's unclear to me why onScriptLoaderFinished also needs to guard against m_askedToTerminate: this seems unsafe by default (if it's true, the dtor has run, and running the callback is unsafe) https://codereview.chromium.org/2423213002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp (right): https://codereview.chromium.org/2423213002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp:174: if (m_askedToTerminate) Similarly, I've added early returns. It feels a bit weird to need these: I'm hoping that not invoking (most of) the successful loading callbacks if a Document hasn't otherwise completed loading during Frame::detach might improve this... https://codereview.chromium.org/2423213002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp:191: WTF::unretained(this))); Same question here: why does onScriptLoaderFinished() also need to check m_askedToTerminate?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
toyoshim@chromium.org changed reviewers: + nhiroki@chromium.org
+nhiroki who owns worker related code
esprehn: ping =)
Does this issue block your project or something? If it blocks nothing, I'd prefer to fix load notifications first, so that we can avoid scattering the guards. What do you think? https://codereview.chromium.org/2423213002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.cpp (right): https://codereview.chromium.org/2423213002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.cpp:356: WTF::unretained(this))); On 2016/10/17 22:33:29, dcheng wrote: > It's unclear to me why onScriptLoaderFinished also needs to guard against > m_askedToTerminate: this seems unsafe by default (if it's true, the dtor has > run, and running the callback is unsafe) Good point. Yeah, that guard looks strange. According to the git history, the guard could be valid until this change: https://codereview.chromium.org/449533003 I think we can replace it with DCHECK(!m_askedToTerminate) or just remove it. https://codereview.chromium.org/2423213002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp (right): https://codereview.chromium.org/2423213002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp:191: WTF::unretained(this))); On 2016/10/17 22:33:29, dcheng wrote: > Same question here: why does onScriptLoaderFinished() also need to check > m_askedToTerminate? ditto(https://codereview.chromium.org/451603002)
kinuko@chromium.org changed reviewers: + kinuko@chromium.org
https://codereview.chromium.org/2423213002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.cpp (right): https://codereview.chromium.org/2423213002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.cpp:356: WTF::unretained(this))); WebViewImpl::close in this class's dtor could synchronously call onScriptLoaderFinished callback, where it might try to delete this again without the guard. I guess probably a better fix could be using a weak ptr and invalidate it at the beginning of the dtor.
kinuko@chromium.org changed reviewers: - kinuko@chromium.org
On 2016/10/20 05:57:12, nhiroki (slow) wrote: > Does this issue block your project or something? If it blocks nothing, I'd > prefer to fix load notifications first, so that we can avoid scattering the > guards. What do you think? > > https://codereview.chromium.org/2423213002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.cpp (right): > > https://codereview.chromium.org/2423213002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.cpp:356: > WTF::unretained(this))); > On 2016/10/17 22:33:29, dcheng wrote: > > It's unclear to me why onScriptLoaderFinished also needs to guard against > > m_askedToTerminate: this seems unsafe by default (if it's true, the dtor has > > run, and running the callback is unsafe) > > Good point. Yeah, that guard looks strange. According to the git history, the > guard could be valid until this change: > https://codereview.chromium.org/449533003 > > I think we can replace it with DCHECK(!m_askedToTerminate) or just remove it. > > https://codereview.chromium.org/2423213002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp (right): > > https://codereview.chromium.org/2423213002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp:191: > WTF::unretained(this))); > On 2016/10/17 22:33:29, dcheng wrote: > > Same question here: why does onScriptLoaderFinished() also need to check > > m_askedToTerminate? > > ditto(https://codereview.chromium.org/451603002) I'm not blocked on this, it's strictly a code health cleanup. I have a patch in progress to suppress loading notifications during detach: all the tests pass, but I need to do some manual verification that the spinner doesn't spin forever in this case. I'll update this CL once I can land the notification suppression.
lgtm, but I'd prefer we return the empty interface provider instead of null. :) Also I wonder if client() should return a reference, and ASSERT() internally that it's never null. Then code that can get called should check some method like !m_webFrame->isActive() and early return instead of null checking client(). That puts an implicit null check at all of the users of client() which is nice. https://codereview.chromium.org/2423213002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp (right): https://codereview.chromium.org/2423213002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp:198: if (m_askedToTerminate) should m_networkProvider be nulled out once this is true? https://codereview.chromium.org/2423213002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp:208: return m_networkProvider->serviceWorkerID(dataSource); ditto? https://codereview.chromium.org/2423213002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp:213: return nullptr; Can we return getEmptyInterfaceProvider() instead? That avoids null checks on interfaceProvider() which should ideally never return null
On 2016/10/24 21:27:08, esprehn wrote: > lgtm, but I'd prefer we return the empty interface provider instead of null. :) > > Also I wonder if client() should return a reference, and ASSERT() internally > that it's never null. Then code that can get called should check some method > like !m_webFrame->isActive() and early return instead of null checking client(). Yep! That's basically the idea I ran by Nate last week, so I'm glad that multiple people feel this is the right long-term approach =) > That puts an implicit null check at all of the users of client() which is nice. > > https://codereview.chromium.org/2423213002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp (right): > > https://codereview.chromium.org/2423213002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp:198: if > (m_askedToTerminate) > should m_networkProvider be nulled out once this is true? > > https://codereview.chromium.org/2423213002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp:208: return > m_networkProvider->serviceWorkerID(dataSource); > ditto? > > https://codereview.chromium.org/2423213002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp:213: return nullptr; > Can we return getEmptyInterfaceProvider() instead? That avoids null checks on > interfaceProvider() which should ideally never return null I'll reply to this CL once I get the loading callbacks CL landed (and hopefully don't need the hacks in the worker code) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
